From f0d36d4a988bdf562a552e6d8d5fbf7c3bbe6406 Mon Sep 17 00:00:00 2001 From: Yonghye Kwon Date: Tue, 28 Jul 2026 18:52:14 +0900 Subject: [PATCH 1/5] fix(analyzer): register KrPassportRecognizer and align its default language KrPassportRecognizer was the only Korean recognizer absent from default_recognizers.yaml, for two reasons that also kept it out of the predefined registry. RecognizerListLoader.get instantiates every predefined recognizer with a name keyword argument, taken from the YAML entry (or from name when class_name supplies the class). KrPassportRecognizer.__init__ did not accept it, so listing the recognizer in default_recognizers.yaml made the registry raise TypeError on load. Adding the argument is what makes the entry possible. Its default supported_language was kr, while every other Korean recognizer defaults to ko, the ISO 639-1 code. #1742 migrated the Korean recognizers from kr to ko; #1814 added this one afterwards and reintroduced kr, so an AnalyzerEngine running ko silently skipped it. The default is now ko. This only affects direct instantiation: when an entry omits supported_languages the loader passes the registry's language explicitly, so the class default never applies on the YAML path. Registers the recognizer with enabled: false and country_code: kr, matching its siblings, and covers all three points with tests. Co-Authored-By: Claude Opus 5 (1M context) --- .../conf/default_recognizers.yaml | 8 +++ .../korea/kr_passport_recognizer.py | 10 ++- .../tests/test_kr_passport_recognizer.py | 70 +++++++++++++++++-- 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml b/presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml index 12791f39aa..fbd844ba46 100644 --- a/presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml +++ b/presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml @@ -311,6 +311,14 @@ recognizers: enabled: false country_code: kr + - name: KrPassportRecognizer + supported_languages: + - ko + - kr + type: predefined + enabled: false + country_code: kr + - name: SeOrganisationsnummerRecognizer supported_languages: - sv diff --git a/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_passport_recognizer.py b/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_passport_recognizer.py index 93606722c8..f8c6db1610 100644 --- a/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_passport_recognizer.py +++ b/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_passport_recognizer.py @@ -18,6 +18,12 @@ class KrPassportRecognizer(PatternRecognizer): the previous passport number format: - one letter 'M' or 'm' or 'S' or 's' or 'R' or 'r' or 'O' or 'o' or 'D' or 'd' - eight digits + + :param patterns: List of patterns to be used by this recognizer + :param context: List of context words to increase confidence in detection + :param supported_language: Language this recognizer supports + :param supported_entity: The entity this recognizer can detect + :param name: Name of the recognizer """ COUNTRY_CODE = "kr" @@ -48,8 +54,9 @@ def __init__( self, patterns: Optional[List[Pattern]] = None, context: Optional[List[str]] = None, - supported_language: str = "kr", + supported_language: str = "ko", supported_entity: str = "KR_PASSPORT", + name: Optional[str] = None, ): patterns = patterns if patterns else self.PATTERNS context = context if context else self.CONTEXT @@ -58,4 +65,5 @@ def __init__( patterns=patterns, context=context, supported_language=supported_language, + name=name, ) diff --git a/presidio-analyzer/tests/test_kr_passport_recognizer.py b/presidio-analyzer/tests/test_kr_passport_recognizer.py index b6d7d93f35..a70615a8e3 100644 --- a/presidio-analyzer/tests/test_kr_passport_recognizer.py +++ b/presidio-analyzer/tests/test_kr_passport_recognizer.py @@ -1,7 +1,17 @@ +import copy +import tempfile +from pathlib import Path + +import presidio_analyzer import pytest +import yaml +from presidio_analyzer.predefined_recognizers.country_specific.korea.kr_passport_recognizer import ( + KrPassportRecognizer, +) +from presidio_analyzer.recognizer_registry import RecognizerRegistryProvider from tests import assert_result_within_score_range -from presidio_analyzer.predefined_recognizers.country_specific.korea.kr_passport_recognizer import KrPassportRecognizer + @pytest.fixture(scope="module") def recognizer(): @@ -25,7 +35,7 @@ def entities(): ("My passport number is M123A4567", 1, ((22, 31),), ((0.1, 0.1),)), ("Korean passport: M456B7890", 1, ((17, 26),), ((0.1, 0.1),)), ("여권번호는 M789C1234입니다", 1, ((6, 15),), ((0.1, 0.1),)), - + # Valid previous format passports (M + 8 digits) ("M12345678", 1, ((0, 9),), ((0.05, 0.05),)), ("m87654321", 1, ((0, 9),), ((0.05, 0.05),)), @@ -33,10 +43,10 @@ def entities(): ("s99887766", 1, ((0, 9),), ((0.05, 0.05),)), ("My old passport M12345678", 1, ((16, 25),), ((0.05, 0.05),)), ("대한민국 여권 S87654321", 1, ((8, 17),), ((0.05, 0.05),)), - + # Multiple passport numbers ("M123A4567 and M456B7890", 2, ((0, 9), (14, 23)), ((0.1, 0.1), (0.1, 0.1))), - + # Invalid formats - should not match ("A123B4567", 0, (), ()), # Wrong first letter ("M12A4567", 0, (), ()), # Too few digits before letter @@ -69,7 +79,7 @@ def test_when_all_passports_then_succeed( assert len(results) == expected_len for res, (st_pos, fn_pos), (st_score, fn_score) in zip( results, expected_positions, expected_score_ranges - ): + ): print(f"res: {res}, st_pos: {st_pos}, fn_pos: {fn_pos}, st_score: {st_score}, fn_score: {fn_score}") if fn_score == "max": fn_score = max_score @@ -104,7 +114,53 @@ def test_when_no_passport_then_no_results(recognizer, entities): "M123456789", # Too long for old format "123A4567", # Missing M prefix ] - + for text in invalid_texts: results = recognizer.analyze(text, entities) - assert len(results) == 0, f"Expected no results for text: {text}" \ No newline at end of file + assert len(results) == 0, f"Expected no results for text: {text}" + + +def test_default_supported_language_is_ko(): + """Default language must be ``ko``, like the other Korean recognizers. + + It was previously ``kr``, which registered the recognizer under a language + code that an AnalyzerEngine running ``ko`` never queried. + """ + assert KrPassportRecognizer().supported_language == "ko" + + +def test_accepts_name_kwarg(): + """Constructor must accept the ``name`` kwarg the YAML loader passes. + + Without it, loading the recognizer from a registry YAML raises + ``TypeError``, which is why it could not be listed there before. + """ + recognizer = KrPassportRecognizer(name="CustomKrPassport") + assert recognizer.name == "CustomKrPassport" + + +@pytest.mark.parametrize("language", ["ko", "kr"]) +def test_loads_from_default_recognizers_yaml(language): + """Recognizer is registered in the default YAML and loads once enabled.""" + conf = ( + Path(presidio_analyzer.__file__).parent + / "conf" + / "default_recognizers.yaml" + ) + recognizers = yaml.safe_load(conf.read_text())["recognizers"] + entries = [r for r in recognizers if r.get("name") == "KrPassportRecognizer"] + assert len(entries) == 1, "KrPassportRecognizer missing from YAML" + entry = entries[0] + assert entry["country_code"] == "kr" + assert language in entry["supported_languages"] + + entry = copy.deepcopy(entry) + entry["enabled"] = True + tmp = Path(tempfile.mkdtemp()) / "conf.yaml" + tmp.write_text( + yaml.safe_dump({"supported_languages": [language], "recognizers": [entry]}) + ) + provider = RecognizerRegistryProvider(conf_file=str(tmp)) + registry = provider.create_recognizer_registry() + entities = {e for rec in registry.recognizers for e in rec.supported_entities} + assert "KR_PASSPORT" in entities From ad36889942ef1c635b8e297007fce3e44d1e41cf Mon Sep 17 00:00:00 2001 From: Yonghye Kwon Date: Tue, 28 Jul 2026 18:52:14 +0900 Subject: [PATCH 2/5] fix(analyzer): make every listed recognizer loadable from the registry YAML Three recognizers ship in default_recognizers.yaml with enabled: false but cannot be turned on. KrBrnRecognizer, KrDriverLicenseRecognizer and UsMbiRecognizer do not accept the name keyword argument that RecognizerListLoader passes to every predefined recognizer, so flipping enabled to true raises TypeError: __init__() got an unexpected keyword argument 'name' before the registry finishes loading. enabled: false is an opt-in switch, not a disclaimer: an entry that cannot be enabled should not be listed. The failure also reads as a user configuration error even though the YAML is correct, unlike the optional-dependency entries, which refuse to load with an actionable ImportError. Adds the argument to the three constructors, and adds contract tests so the next one is caught by CI. Why this survived: the constructor signature is part of a contract that nothing enforced. Each recognizer's own tests instantiate the class directly, where no name is passed, so they all pass. The registry-level tests build the default configuration, in which roughly 60 entries are disabled and therefore never constructed. Nothing in between ever looked. The new tests close that gap from both sides: - Every predefined PatternRecognizer subclass must accept the kwargs the loader passes. This fires when the class is added, before it reaches the YAML at all, which is the point at which KrPassportRecognizer went wrong in #1814. - Every entry in default_recognizers.yaml must resolve to a class and must load once enabled, exercised entry by entry so a failure names the recognizer. - A class_name plus name entry must produce an instance with the configured name, which is the documented reason the loader passes name and what makes the kwarg contract load-bearing. Verified by reverting the three constructors: the signature test and the load test each fail for exactly those three, with no other failures. One entry is excluded from the load test by name, with its reason recorded next to the exclusion, and a further test asserts each exclusion still matches a shipped entry so the list cannot rot: - HuggingFaceNerRecognizer cannot load from its shipped entry even with its dependencies installed. EntityRecognizer.__init__ calls load() unconditionally, load() requires model_name, and the entry does not supply one, so it raises ValueError rather than the ImportError it raises when transformers is absent. Supplying model_name here would make the test download a model. The entry stays covered by the resolve test. The load test skips only BasicLangExtractRecognizer, and only on ImportError, because refusing to load without the langextract extra is that recognizer's intended behavior. The skip is scoped to that name rather than to the exception type, so an ImportError from any other entry is a failure instead of a green skip. BasicLangExtractRecognizer also carries a config_path that the recognizer resolves against the working directory, so it raises FileNotFoundError when pytest runs from the repository root. The load test sets the working directory to the component root, the same one CI uses, instead of catching that error: catching FileNotFoundError would turn a deleted or renamed shipped config file into a passing skip. A separate test asserts every config_path in the shipped configuration resolves to a file that exists, which holds even in an environment that cannot construct the recognizer at all. The load test deliberately covers non-pattern entries too. Narrowing it to PatternRecognizer subclasses would silently drop PhoneRecognizer, ZaMobileNumberRecognizer and ZaTelephoneNumberRecognizer, which are not PatternRecognizers and do load. The signature test is scoped to PatternRecognizer subclasses. AzureAILanguageRecognizer is the one remaining class that does not accept name; it is absent from default_recognizers.yaml and fixes its own display name, so it is out of scope here, but it is reachable from a user config by class name and fails the same way. That is pre-existing and tracked separately. Co-Authored-By: Claude Opus 5 (1M context) --- .../korea/kr_brn_recognizer.py | 2 + .../korea/kr_driver_license_recognizer.py | 2 + .../country_specific/us/us_mbi_recognizer.py | 2 + .../test_predefined_recognizer_contract.py | 238 ++++++++++++++++++ 4 files changed, 244 insertions(+) create mode 100644 presidio-analyzer/tests/test_predefined_recognizer_contract.py diff --git a/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_brn_recognizer.py b/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_brn_recognizer.py index e1c4af913f..5a7f40f02a 100644 --- a/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_brn_recognizer.py +++ b/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_brn_recognizer.py @@ -58,6 +58,7 @@ def __init__( supported_language: str = "ko", supported_entity: str = "KR_BRN", replacement_pairs: Optional[List[Tuple[str, str]]] = None, + name: Optional[str] = None, ): self.replacement_pairs = replacement_pairs if replacement_pairs else [("-", "")] @@ -68,6 +69,7 @@ def __init__( patterns=patterns, context=context, supported_language=supported_language, + name=name, ) def validate_result(self, pattern_text: str) -> Union[bool, None]: diff --git a/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_driver_license_recognizer.py b/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_driver_license_recognizer.py index 16dffe35d4..7ba14d2227 100644 --- a/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_driver_license_recognizer.py +++ b/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_driver_license_recognizer.py @@ -70,6 +70,7 @@ def __init__( supported_language: str = "ko", supported_entity: str = "KR_DRIVER_LICENSE", replacement_pairs: Optional[List[Tuple[str, str]]] = None, + name: Optional[str] = None, ): self.replacement_pairs = ( replacement_pairs if replacement_pairs else [("-", ""), (" ", "")] @@ -82,6 +83,7 @@ def __init__( patterns=patterns, context=context, supported_language=supported_language, + name=name, ) def validate_result(self, pattern_text: str) -> bool: diff --git a/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/us/us_mbi_recognizer.py b/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/us/us_mbi_recognizer.py index 8d4c391e2d..718a2f89f7 100644 --- a/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/us/us_mbi_recognizer.py +++ b/presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/us/us_mbi_recognizer.py @@ -90,6 +90,7 @@ def __init__( context: Optional[List[str]] = None, supported_language: str = "en", supported_entity: str = "US_MBI", + name: Optional[str] = None, ): patterns = patterns if patterns else self.PATTERNS context = context if context else self.CONTEXT @@ -98,4 +99,5 @@ def __init__( patterns=patterns, context=context, supported_language=supported_language, + name=name, ) diff --git a/presidio-analyzer/tests/test_predefined_recognizer_contract.py b/presidio-analyzer/tests/test_predefined_recognizer_contract.py new file mode 100644 index 0000000000..7cfedc13bd --- /dev/null +++ b/presidio-analyzer/tests/test_predefined_recognizer_contract.py @@ -0,0 +1,238 @@ +"""Contract tests between the registry loader and predefined recognizers. + +``RecognizerListLoader`` builds predefined recognizers from YAML by passing the +entry's keys as constructor kwargs. That makes the constructor signature part of +a contract which nothing else enforces: a recognizer can satisfy every one of +its own unit tests -- which instantiate it directly -- and still be impossible +to load from a registry configuration. + +The gap is specifically in the *disabled* entries. ``default_recognizers.yaml`` +ships ~60 recognizers with ``enabled: false``, and no other test constructs +them, so a broken constructor stays invisible until a user flips the switch. +These tests construct every one of them. +""" + +import inspect +from pathlib import Path +from typing import Dict, List + +import presidio_analyzer.predefined_recognizers as predefined +import pytest +import yaml +from presidio_analyzer import EntityRecognizer, PatternRecognizer +from presidio_analyzer.recognizer_registry import RecognizerRegistryProvider +from presidio_analyzer.recognizer_registry.recognizers_loader_utils import ( + RecognizerListLoader, +) + +# The component root, i.e. the directory that contains the ``presidio_analyzer`` +# package. Some shipped entries carry a ``config_path`` that the recognizer +# resolves relative to the current working directory, so the load test runs from +# here -- the same working directory CI uses -- rather than catching the +# resulting error. Catching it would turn a genuinely missing shipped file into +# a passing skip. +PACKAGE_ROOT = Path(__file__).resolve().parent.parent + +DEFAULT_CONF = PACKAGE_ROOT / "presidio_analyzer" / "conf" / "default_recognizers.yaml" + +# Kwargs ``RecognizerListLoader`` passes to every predefined recognizer it +# builds: ``name`` comes from the YAML entry (or its ``class_name`` alias) and +# ``supported_language`` from the resolved per-language configuration. +LOADER_KWARGS = ("name", "supported_language") + +# Entries that cannot load from their shipped configuration even with every +# dependency installed, so the load test below cannot cover them. +# +# ``HuggingFaceNerRecognizer``: ``EntityRecognizer.__init__`` calls ``load()`` +# unconditionally and ``load()`` requires ``model_name``, which the shipped +# entry does not supply -- it raises ValueError once ``transformers`` and +# ``torch`` are present. That is a pre-existing defect in the entry, not +# something this contract can assert away, and adding ``model_name`` here would +# make the test download a model. It stays covered by the resolve test. +NOT_LOADABLE_FROM_SHIPPED_ENTRY = {"HuggingFaceNerRecognizer"} + +# Entries gated behind an optional dependency, for which refusing to load with an +# actionable ImportError is the intended behavior. The skip is scoped to these +# names rather than to the exception type, so an ImportError from any other entry +# stays a failure instead of a green skip. +OPTIONAL_DEPENDENCY_ENTRIES = {"BasicLangExtractRecognizer"} + + +def _pattern_recognizer_classes() -> Dict[str, type]: + """Predefined ``PatternRecognizer`` subclasses, which the YAML loader builds. + + Non-pattern recognizers (NER/LLM/remote wrappers) are excluded: several are + not registrable from ``default_recognizers.yaml`` and some deliberately fix + their own display name. + """ + classes = {} + for attr in dir(predefined): + obj = getattr(predefined, attr) + if not isinstance(obj, type) or not issubclass(obj, EntityRecognizer): + continue + if obj in (EntityRecognizer, PatternRecognizer): + continue + if issubclass(obj, PatternRecognizer): + classes[attr] = obj + return classes + + +def _yaml_entries() -> List[Dict]: + """Normalize the shipped recognizer list to dict entries.""" + data = yaml.safe_load(DEFAULT_CONF.read_text(encoding="utf-8")) + entries = [] + for entry in data["recognizers"]: + entries.append({"name": entry} if isinstance(entry, str) else dict(entry)) + return entries + + +def _entry_languages(entry: Dict) -> List[str]: + """Languages an entry declares, in either supported YAML shape.""" + languages = entry.get("supported_languages") + if not languages: + return ["en"] + if isinstance(languages[0], str): + return list(languages) + return [item["language"] for item in languages] + + +def _entry_id(entry: Dict) -> str: + return entry.get("class_name") or entry["name"] + + +PATTERN_CLASSES = _pattern_recognizer_classes() +YAML_ENTRIES = _yaml_entries() +LOADABLE_YAML_ENTRIES = [ + entry + for entry in YAML_ENTRIES + if _entry_id(entry) not in NOT_LOADABLE_FROM_SHIPPED_ENTRY +] + + +def test_default_conf_has_entries(): + """Guard the fixtures themselves: an empty parse would pass everything.""" + assert PATTERN_CLASSES, "no predefined PatternRecognizer subclasses found" + assert YAML_ENTRIES, "no recognizers parsed from default_recognizers.yaml" + + +@pytest.mark.parametrize( + "entry_id", + sorted(NOT_LOADABLE_FROM_SHIPPED_ENTRY | OPTIONAL_DEPENDENCY_ENTRIES), +) +def test_exclusion_names_a_real_entry(entry_id): + """An exclusion must still match a shipped entry. + + Keeps the two lists above from rotting: if an entry is renamed, removed, or + fixed, the stale exclusion fails here instead of silently narrowing + coverage. + """ + assert entry_id in {_entry_id(entry) for entry in YAML_ENTRIES} + + +CONFIG_PATH_ENTRIES = [entry for entry in YAML_ENTRIES if entry.get("config_path")] + + +@pytest.mark.parametrize("entry", CONFIG_PATH_ENTRIES, ids=_entry_id) +def test_yaml_entry_config_path_points_at_a_shipped_file(entry): + """A ``config_path`` in a shipped entry must point at a file that ships. + + Asserted directly rather than left to the load test, which skips the entry + when its optional dependency is absent. A deleted or renamed config file is + a regression that must fail even in an environment that cannot construct the + recognizer at all. + """ + config_path = Path(entry["config_path"]) + resolved = config_path if config_path.is_absolute() else PACKAGE_ROOT / config_path + assert resolved.is_file(), ( + f"{_entry_id(entry)} declares config_path {entry['config_path']!r}, " + f"which does not resolve to a file ({resolved})" + ) + + +@pytest.mark.parametrize("class_name", sorted(PATTERN_CLASSES)) +def test_pattern_recognizer_accepts_loader_kwargs(class_name): + """Constructor must accept every kwarg the YAML loader passes. + + Catches the defect before the recognizer reaches the YAML at all: a class + added without ``name`` passes its own unit tests, and only fails once + someone tries to register it. + """ + parameters = inspect.signature(PATTERN_CLASSES[class_name].__init__).parameters + if any(p.kind == inspect.Parameter.VAR_KEYWORD for p in parameters.values()): + return + missing = [kwarg for kwarg in LOADER_KWARGS if kwarg not in parameters] + assert not missing, ( + f"{class_name}.__init__ does not accept {missing}, which " + f"RecognizerListLoader passes to every predefined recognizer. " + f"Loading it from a registry YAML raises TypeError." + ) + + +@pytest.mark.parametrize("entry", YAML_ENTRIES, ids=_entry_id) +def test_yaml_entry_class_resolves(entry): + """Every shipped entry must name a real recognizer class.""" + RecognizerListLoader.get_existing_recognizer_cls(recognizer_name=_entry_id(entry)) + + +@pytest.mark.parametrize("entry", LOADABLE_YAML_ENTRIES, ids=_entry_id) +def test_yaml_entry_loads_when_enabled(entry, monkeypatch): + """Every shipped entry must load once ``enabled`` is true. + + ``enabled: false`` is an opt-in switch, not a disclaimer -- an entry that + cannot be turned on should not be listed. + + Runs from ``PACKAGE_ROOT`` so that a ``config_path`` the recognizer resolves + against the working directory behaves as it does in CI. A FileNotFoundError + is therefore a real missing shipped file and is left to fail. + """ + entry_id = _entry_id(entry) + entry = dict(entry, enabled=True) + monkeypatch.chdir(PACKAGE_ROOT) + configuration = { + "global_regex_flags": yaml.safe_load(DEFAULT_CONF.read_text(encoding="utf-8"))[ + "global_regex_flags" + ], + "supported_languages": _entry_languages(entry), + "recognizers": [entry], + } + + try: + registry = RecognizerRegistryProvider( + registry_configuration=configuration + ).create_recognizer_registry() + except ImportError as exc: + if entry_id not in OPTIONAL_DEPENDENCY_ENTRIES: + raise + pytest.skip(f"{entry_id} needs an optional dependency: {exc}") + + assert registry.recognizers, ( + f"{_entry_id(entry)} is listed in default_recognizers.yaml but loaded " + f"nothing for languages {_entry_languages(entry)}" + ) + + +def test_yaml_entry_can_be_renamed_via_class_name(): + """``class_name`` + ``name`` must give the instance the configured name. + + This is the documented reason the loader passes ``name`` at all (see + ``RecognizerListLoader.get_recognizer_name``), so it is the behavior that + makes the kwarg contract above load-bearing rather than incidental. + """ + configuration = { + "global_regex_flags": 26, + "supported_languages": ["en"], + "recognizers": [ + { + "class_name": "UsSsnRecognizer", + "name": "MyRenamedSsnRecognizer", + "supported_languages": ["en"], + "type": "predefined", + "country_code": "us", + } + ], + } + registry = RecognizerRegistryProvider( + registry_configuration=configuration + ).create_recognizer_registry() + + assert [r.name for r in registry.recognizers] == ["MyRenamedSsnRecognizer"] From 8c22f473161231ee527810ca4bb1944f0c6e5a93 Mon Sep 17 00:00:00 2001 From: Yonghye Kwon Date: Tue, 4 Aug 2026 20:01:57 +0900 Subject: [PATCH 3/5] test(analyzer): tighten the recognizer contract tests Addresses the review on #2170. The KR passport YAML load test wrote the entry to a `tempfile.mkdtemp()` directory that nothing removed. Rather than wrap it in a context manager, the temporary file is gone: `RecognizerRegistryProvider` accepts the parsed mapping via `registry_configuration`, which is what the sibling contract test in this PR already does, so the round trip through the filesystem was never needed. Its assertion also only checked that `KR_PASSPORT` was among the loaded entities. Both tests now assert on the loaded recognizer *class*: the loader drops a recognizer whose language the registry does not support with a log warning and no exception, so a non-empty registry does not on its own prove the entry under test is what loaded. Also in this commit: - Removed a leftover debug `print` from the parametrized passport test. It predates this PR -- stripping trailing whitespace on the line above pulled it into the diff -- but it is adjacent to code this PR touches. - `default_recognizers.yaml` is parsed once at import instead of once per parametrized load test, which re-read it for the same two keys. - A `**kwargs` constructor now reports `pytest.skip` with a reason rather than returning as a silent pass, so the gap stays visible. No shipped `PatternRecognizer` subclass takes `**kwargs` today. - `test_yaml_entry_class_resolves` asserts the resolved object is an `EntityRecognizer` subclass instead of relying on the lookup raising. - Dropped the entry count from the module docstring. It was accurate but drifts every time a recognizer is added. --- .../tests/test_kr_passport_recognizer.py | 38 +++++++------ .../test_predefined_recognizer_contract.py | 55 +++++++++++++------ 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/presidio-analyzer/tests/test_kr_passport_recognizer.py b/presidio-analyzer/tests/test_kr_passport_recognizer.py index a70615a8e3..a51914ee17 100644 --- a/presidio-analyzer/tests/test_kr_passport_recognizer.py +++ b/presidio-analyzer/tests/test_kr_passport_recognizer.py @@ -1,5 +1,3 @@ -import copy -import tempfile from pathlib import Path import presidio_analyzer @@ -80,7 +78,6 @@ def test_when_all_passports_then_succeed( for res, (st_pos, fn_pos), (st_score, fn_score) in zip( results, expected_positions, expected_score_ranges ): - print(f"res: {res}, st_pos: {st_pos}, fn_pos: {fn_pos}, st_score: {st_score}, fn_score: {fn_score}") if fn_score == "max": fn_score = max_score assert_result_within_score_range( @@ -141,26 +138,31 @@ def test_accepts_name_kwarg(): @pytest.mark.parametrize("language", ["ko", "kr"]) def test_loads_from_default_recognizers_yaml(language): - """Recognizer is registered in the default YAML and loads once enabled.""" - conf = ( - Path(presidio_analyzer.__file__).parent - / "conf" - / "default_recognizers.yaml" - ) - recognizers = yaml.safe_load(conf.read_text())["recognizers"] + """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. + """ + conf = Path(presidio_analyzer.__file__).parent / "conf" / "default_recognizers.yaml" + recognizers = yaml.safe_load(conf.read_text(encoding="utf-8"))["recognizers"] entries = [r for r in recognizers if r.get("name") == "KrPassportRecognizer"] assert len(entries) == 1, "KrPassportRecognizer missing from YAML" entry = entries[0] assert entry["country_code"] == "kr" assert language in entry["supported_languages"] - entry = copy.deepcopy(entry) - entry["enabled"] = True - tmp = Path(tempfile.mkdtemp()) / "conf.yaml" - tmp.write_text( - yaml.safe_dump({"supported_languages": [language], "recognizers": [entry]}) - ) - provider = RecognizerRegistryProvider(conf_file=str(tmp)) - registry = provider.create_recognizer_registry() + # Handed to the provider in memory rather than written to a temporary file: + # ``registry_configuration`` takes the same mapping the YAML parses to, so a + # round trip through the filesystem would only add a directory to clean up. + registry = RecognizerRegistryProvider( + registry_configuration={ + "supported_languages": [language], + "recognizers": [dict(entry, enabled=True)], + } + ).create_recognizer_registry() + + assert [type(r).__name__ for r in registry.recognizers] == ["KrPassportRecognizer"] entities = {e for rec in registry.recognizers for e in rec.supported_entities} assert "KR_PASSPORT" in entities diff --git a/presidio-analyzer/tests/test_predefined_recognizer_contract.py b/presidio-analyzer/tests/test_predefined_recognizer_contract.py index 7cfedc13bd..dc752ae4de 100644 --- a/presidio-analyzer/tests/test_predefined_recognizer_contract.py +++ b/presidio-analyzer/tests/test_predefined_recognizer_contract.py @@ -6,10 +6,10 @@ its own unit tests -- which instantiate it directly -- and still be impossible to load from a registry configuration. -The gap is specifically in the *disabled* entries. ``default_recognizers.yaml`` -ships ~60 recognizers with ``enabled: false``, and no other test constructs -them, so a broken constructor stays invisible until a user flips the switch. -These tests construct every one of them. +The gap is specifically in the *disabled* entries. Most of what +``default_recognizers.yaml`` ships is ``enabled: false``, and no other test +constructs those, so a broken constructor stays invisible until a user flips the +switch. These tests construct every one of them. """ import inspect @@ -35,6 +35,12 @@ DEFAULT_CONF = PACKAGE_ROOT / "presidio_analyzer" / "conf" / "default_recognizers.yaml" +# Parsed once at import. The load test below is parametrized over every shipped +# entry, so re-reading the file per invocation would parse it a few dozen times +# to retrieve the same two keys. +DEFAULT_CONF_DATA = yaml.safe_load(DEFAULT_CONF.read_text(encoding="utf-8")) +GLOBAL_REGEX_FLAGS = DEFAULT_CONF_DATA["global_regex_flags"] + # Kwargs ``RecognizerListLoader`` passes to every predefined recognizer it # builds: ``name`` comes from the YAML entry (or its ``class_name`` alias) and # ``supported_language`` from the resolved per-language configuration. @@ -79,9 +85,8 @@ def _pattern_recognizer_classes() -> Dict[str, type]: def _yaml_entries() -> List[Dict]: """Normalize the shipped recognizer list to dict entries.""" - data = yaml.safe_load(DEFAULT_CONF.read_text(encoding="utf-8")) entries = [] - for entry in data["recognizers"]: + for entry in DEFAULT_CONF_DATA["recognizers"]: entries.append({"name": entry} if isinstance(entry, str) else dict(entry)) return entries @@ -159,7 +164,11 @@ def test_pattern_recognizer_accepts_loader_kwargs(class_name): """ parameters = inspect.signature(PATTERN_CLASSES[class_name].__init__).parameters if any(p.kind == inspect.Parameter.VAR_KEYWORD for p in parameters.values()): - return + pytest.skip( + f"{class_name}.__init__ takes **kwargs, so the signature cannot show " + f"which kwargs it honors. Reported as a skip rather than a silent " + f"pass so the gap in coverage stays visible." + ) missing = [kwarg for kwarg in LOADER_KWARGS if kwarg not in parameters] assert not missing, ( f"{class_name}.__init__ does not accept {missing}, which " @@ -170,8 +179,18 @@ def test_pattern_recognizer_accepts_loader_kwargs(class_name): @pytest.mark.parametrize("entry", YAML_ENTRIES, ids=_entry_id) def test_yaml_entry_class_resolves(entry): - """Every shipped entry must name a real recognizer class.""" - RecognizerListLoader.get_existing_recognizer_cls(recognizer_name=_entry_id(entry)) + """Every shipped entry must name a real recognizer class. + + The resolved object is asserted to be a recognizer class rather than left to + the lookup raising, so an entry that resolves to some unrelated module + attribute fails here instead of downstream. + """ + recognizer_cls = RecognizerListLoader.get_existing_recognizer_cls( + recognizer_name=_entry_id(entry) + ) + assert isinstance(recognizer_cls, type) and issubclass( + recognizer_cls, EntityRecognizer + ), f"{_entry_id(entry)} resolves to {recognizer_cls!r}, not a recognizer class" @pytest.mark.parametrize("entry", LOADABLE_YAML_ENTRIES, ids=_entry_id) @@ -186,13 +205,12 @@ def test_yaml_entry_loads_when_enabled(entry, monkeypatch): is therefore a real missing shipped file and is left to fail. """ entry_id = _entry_id(entry) + languages = _entry_languages(entry) entry = dict(entry, enabled=True) monkeypatch.chdir(PACKAGE_ROOT) configuration = { - "global_regex_flags": yaml.safe_load(DEFAULT_CONF.read_text(encoding="utf-8"))[ - "global_regex_flags" - ], - "supported_languages": _entry_languages(entry), + "global_regex_flags": GLOBAL_REGEX_FLAGS, + "supported_languages": languages, "recognizers": [entry], } @@ -205,9 +223,14 @@ def test_yaml_entry_loads_when_enabled(entry, monkeypatch): raise pytest.skip(f"{entry_id} needs an optional dependency: {exc}") - assert registry.recognizers, ( - f"{_entry_id(entry)} is listed in default_recognizers.yaml but loaded " - f"nothing for languages {_entry_languages(entry)}" + # Asserted on the class rather than on ``registry.recognizers`` being + # non-empty: the loader drops a recognizer whose language the registry does + # not support with a log warning and no exception, so a merely non-empty + # registry would not prove that *this* entry is what loaded. + loaded = [type(r).__name__ for r in registry.recognizers] + assert entry_id in loaded, ( + f"{entry_id} is listed in default_recognizers.yaml but loaded " + f"nothing for languages {languages} (registry holds {loaded})" ) From dc9b3ff95dabc651669f6191af683b1214c88762 Mon Sep 17 00:00:00 2001 From: Yonghye Kwon Date: Wed, 5 Aug 2026 09:13:02 +0900 Subject: [PATCH 4/5] test(analyzer): fold the contract tests into the loader test file Review feedback: tests in this repo are arranged by the file they test, so a standalone test_predefined_recognizer_contract.py is the wrong shape. Moved into tests/test_recognizers_loader_utils.py, which covers recognizers_loader_utils.py -- the module whose contract these assert. RecognizerListLoader is what passes `name`/`supported_language` to every predefined recognizer, what resolves an entry's class, and what consumes default_recognizers.yaml. That file already hosts the sibling shipped-YAML check (test_default_recognizers_yaml_country_code_matches_class), so the two now sit together instead of in separate files. The tests themselves are unchanged; the module docstring became a section header. test_default_recognizers_yaml_country_code_matches_class now reads the shipped YAML through the module-level constant the move introduced, rather than re-opening the same file a second time in one module. Test count is unchanged at 295 (294 pass, 1 skip -- BasicLangExtract, no optional dependency installed). Co-Authored-By: Claude Opus 5 (1M context) --- .../test_predefined_recognizer_contract.py | 261 ----------------- .../tests/test_recognizers_loader_utils.py | 271 +++++++++++++++++- 2 files changed, 262 insertions(+), 270 deletions(-) delete mode 100644 presidio-analyzer/tests/test_predefined_recognizer_contract.py diff --git a/presidio-analyzer/tests/test_predefined_recognizer_contract.py b/presidio-analyzer/tests/test_predefined_recognizer_contract.py deleted file mode 100644 index dc752ae4de..0000000000 --- a/presidio-analyzer/tests/test_predefined_recognizer_contract.py +++ /dev/null @@ -1,261 +0,0 @@ -"""Contract tests between the registry loader and predefined recognizers. - -``RecognizerListLoader`` builds predefined recognizers from YAML by passing the -entry's keys as constructor kwargs. That makes the constructor signature part of -a contract which nothing else enforces: a recognizer can satisfy every one of -its own unit tests -- which instantiate it directly -- and still be impossible -to load from a registry configuration. - -The gap is specifically in the *disabled* entries. Most of what -``default_recognizers.yaml`` ships is ``enabled: false``, and no other test -constructs those, so a broken constructor stays invisible until a user flips the -switch. These tests construct every one of them. -""" - -import inspect -from pathlib import Path -from typing import Dict, List - -import presidio_analyzer.predefined_recognizers as predefined -import pytest -import yaml -from presidio_analyzer import EntityRecognizer, PatternRecognizer -from presidio_analyzer.recognizer_registry import RecognizerRegistryProvider -from presidio_analyzer.recognizer_registry.recognizers_loader_utils import ( - RecognizerListLoader, -) - -# The component root, i.e. the directory that contains the ``presidio_analyzer`` -# package. Some shipped entries carry a ``config_path`` that the recognizer -# resolves relative to the current working directory, so the load test runs from -# here -- the same working directory CI uses -- rather than catching the -# resulting error. Catching it would turn a genuinely missing shipped file into -# a passing skip. -PACKAGE_ROOT = Path(__file__).resolve().parent.parent - -DEFAULT_CONF = PACKAGE_ROOT / "presidio_analyzer" / "conf" / "default_recognizers.yaml" - -# Parsed once at import. The load test below is parametrized over every shipped -# entry, so re-reading the file per invocation would parse it a few dozen times -# to retrieve the same two keys. -DEFAULT_CONF_DATA = yaml.safe_load(DEFAULT_CONF.read_text(encoding="utf-8")) -GLOBAL_REGEX_FLAGS = DEFAULT_CONF_DATA["global_regex_flags"] - -# Kwargs ``RecognizerListLoader`` passes to every predefined recognizer it -# builds: ``name`` comes from the YAML entry (or its ``class_name`` alias) and -# ``supported_language`` from the resolved per-language configuration. -LOADER_KWARGS = ("name", "supported_language") - -# Entries that cannot load from their shipped configuration even with every -# dependency installed, so the load test below cannot cover them. -# -# ``HuggingFaceNerRecognizer``: ``EntityRecognizer.__init__`` calls ``load()`` -# unconditionally and ``load()`` requires ``model_name``, which the shipped -# entry does not supply -- it raises ValueError once ``transformers`` and -# ``torch`` are present. That is a pre-existing defect in the entry, not -# something this contract can assert away, and adding ``model_name`` here would -# make the test download a model. It stays covered by the resolve test. -NOT_LOADABLE_FROM_SHIPPED_ENTRY = {"HuggingFaceNerRecognizer"} - -# Entries gated behind an optional dependency, for which refusing to load with an -# actionable ImportError is the intended behavior. The skip is scoped to these -# names rather than to the exception type, so an ImportError from any other entry -# stays a failure instead of a green skip. -OPTIONAL_DEPENDENCY_ENTRIES = {"BasicLangExtractRecognizer"} - - -def _pattern_recognizer_classes() -> Dict[str, type]: - """Predefined ``PatternRecognizer`` subclasses, which the YAML loader builds. - - Non-pattern recognizers (NER/LLM/remote wrappers) are excluded: several are - not registrable from ``default_recognizers.yaml`` and some deliberately fix - their own display name. - """ - classes = {} - for attr in dir(predefined): - obj = getattr(predefined, attr) - if not isinstance(obj, type) or not issubclass(obj, EntityRecognizer): - continue - if obj in (EntityRecognizer, PatternRecognizer): - continue - if issubclass(obj, PatternRecognizer): - classes[attr] = obj - return classes - - -def _yaml_entries() -> List[Dict]: - """Normalize the shipped recognizer list to dict entries.""" - entries = [] - for entry in DEFAULT_CONF_DATA["recognizers"]: - entries.append({"name": entry} if isinstance(entry, str) else dict(entry)) - return entries - - -def _entry_languages(entry: Dict) -> List[str]: - """Languages an entry declares, in either supported YAML shape.""" - languages = entry.get("supported_languages") - if not languages: - return ["en"] - if isinstance(languages[0], str): - return list(languages) - return [item["language"] for item in languages] - - -def _entry_id(entry: Dict) -> str: - return entry.get("class_name") or entry["name"] - - -PATTERN_CLASSES = _pattern_recognizer_classes() -YAML_ENTRIES = _yaml_entries() -LOADABLE_YAML_ENTRIES = [ - entry - for entry in YAML_ENTRIES - if _entry_id(entry) not in NOT_LOADABLE_FROM_SHIPPED_ENTRY -] - - -def test_default_conf_has_entries(): - """Guard the fixtures themselves: an empty parse would pass everything.""" - assert PATTERN_CLASSES, "no predefined PatternRecognizer subclasses found" - assert YAML_ENTRIES, "no recognizers parsed from default_recognizers.yaml" - - -@pytest.mark.parametrize( - "entry_id", - sorted(NOT_LOADABLE_FROM_SHIPPED_ENTRY | OPTIONAL_DEPENDENCY_ENTRIES), -) -def test_exclusion_names_a_real_entry(entry_id): - """An exclusion must still match a shipped entry. - - Keeps the two lists above from rotting: if an entry is renamed, removed, or - fixed, the stale exclusion fails here instead of silently narrowing - coverage. - """ - assert entry_id in {_entry_id(entry) for entry in YAML_ENTRIES} - - -CONFIG_PATH_ENTRIES = [entry for entry in YAML_ENTRIES if entry.get("config_path")] - - -@pytest.mark.parametrize("entry", CONFIG_PATH_ENTRIES, ids=_entry_id) -def test_yaml_entry_config_path_points_at_a_shipped_file(entry): - """A ``config_path`` in a shipped entry must point at a file that ships. - - Asserted directly rather than left to the load test, which skips the entry - when its optional dependency is absent. A deleted or renamed config file is - a regression that must fail even in an environment that cannot construct the - recognizer at all. - """ - config_path = Path(entry["config_path"]) - resolved = config_path if config_path.is_absolute() else PACKAGE_ROOT / config_path - assert resolved.is_file(), ( - f"{_entry_id(entry)} declares config_path {entry['config_path']!r}, " - f"which does not resolve to a file ({resolved})" - ) - - -@pytest.mark.parametrize("class_name", sorted(PATTERN_CLASSES)) -def test_pattern_recognizer_accepts_loader_kwargs(class_name): - """Constructor must accept every kwarg the YAML loader passes. - - Catches the defect before the recognizer reaches the YAML at all: a class - added without ``name`` passes its own unit tests, and only fails once - someone tries to register it. - """ - parameters = inspect.signature(PATTERN_CLASSES[class_name].__init__).parameters - if any(p.kind == inspect.Parameter.VAR_KEYWORD for p in parameters.values()): - pytest.skip( - f"{class_name}.__init__ takes **kwargs, so the signature cannot show " - f"which kwargs it honors. Reported as a skip rather than a silent " - f"pass so the gap in coverage stays visible." - ) - missing = [kwarg for kwarg in LOADER_KWARGS if kwarg not in parameters] - assert not missing, ( - f"{class_name}.__init__ does not accept {missing}, which " - f"RecognizerListLoader passes to every predefined recognizer. " - f"Loading it from a registry YAML raises TypeError." - ) - - -@pytest.mark.parametrize("entry", YAML_ENTRIES, ids=_entry_id) -def test_yaml_entry_class_resolves(entry): - """Every shipped entry must name a real recognizer class. - - The resolved object is asserted to be a recognizer class rather than left to - the lookup raising, so an entry that resolves to some unrelated module - attribute fails here instead of downstream. - """ - recognizer_cls = RecognizerListLoader.get_existing_recognizer_cls( - recognizer_name=_entry_id(entry) - ) - assert isinstance(recognizer_cls, type) and issubclass( - recognizer_cls, EntityRecognizer - ), f"{_entry_id(entry)} resolves to {recognizer_cls!r}, not a recognizer class" - - -@pytest.mark.parametrize("entry", LOADABLE_YAML_ENTRIES, ids=_entry_id) -def test_yaml_entry_loads_when_enabled(entry, monkeypatch): - """Every shipped entry must load once ``enabled`` is true. - - ``enabled: false`` is an opt-in switch, not a disclaimer -- an entry that - cannot be turned on should not be listed. - - Runs from ``PACKAGE_ROOT`` so that a ``config_path`` the recognizer resolves - against the working directory behaves as it does in CI. A FileNotFoundError - is therefore a real missing shipped file and is left to fail. - """ - entry_id = _entry_id(entry) - languages = _entry_languages(entry) - entry = dict(entry, enabled=True) - monkeypatch.chdir(PACKAGE_ROOT) - configuration = { - "global_regex_flags": GLOBAL_REGEX_FLAGS, - "supported_languages": languages, - "recognizers": [entry], - } - - try: - registry = RecognizerRegistryProvider( - registry_configuration=configuration - ).create_recognizer_registry() - except ImportError as exc: - if entry_id not in OPTIONAL_DEPENDENCY_ENTRIES: - raise - pytest.skip(f"{entry_id} needs an optional dependency: {exc}") - - # Asserted on the class rather than on ``registry.recognizers`` being - # non-empty: the loader drops a recognizer whose language the registry does - # not support with a log warning and no exception, so a merely non-empty - # registry would not prove that *this* entry is what loaded. - loaded = [type(r).__name__ for r in registry.recognizers] - assert entry_id in loaded, ( - f"{entry_id} is listed in default_recognizers.yaml but loaded " - f"nothing for languages {languages} (registry holds {loaded})" - ) - - -def test_yaml_entry_can_be_renamed_via_class_name(): - """``class_name`` + ``name`` must give the instance the configured name. - - This is the documented reason the loader passes ``name`` at all (see - ``RecognizerListLoader.get_recognizer_name``), so it is the behavior that - makes the kwarg contract above load-bearing rather than incidental. - """ - configuration = { - "global_regex_flags": 26, - "supported_languages": ["en"], - "recognizers": [ - { - "class_name": "UsSsnRecognizer", - "name": "MyRenamedSsnRecognizer", - "supported_languages": ["en"], - "type": "predefined", - "country_code": "us", - } - ], - } - registry = RecognizerRegistryProvider( - registry_configuration=configuration - ).create_recognizer_registry() - - assert [r.name for r in registry.recognizers] == ["MyRenamedSsnRecognizer"] diff --git a/presidio-analyzer/tests/test_recognizers_loader_utils.py b/presidio-analyzer/tests/test_recognizers_loader_utils.py index 7a5bf2b7cd..bb222aa229 100644 --- a/presidio-analyzer/tests/test_recognizers_loader_utils.py +++ b/presidio-analyzer/tests/test_recognizers_loader_utils.py @@ -2,21 +2,41 @@ import copy import functools +import inspect import re from pathlib import Path +from typing import Dict, List +import presidio_analyzer.predefined_recognizers as predefined import pytest import yaml -from presidio_analyzer import Pattern, PatternRecognizer +from presidio_analyzer import EntityRecognizer, Pattern, PatternRecognizer from presidio_analyzer.predefined_recognizers import ( CreditCardRecognizer, UsSsnRecognizer, ) +from presidio_analyzer.recognizer_registry import RecognizerRegistryProvider from presidio_analyzer.recognizer_registry.recognizers_loader_utils import ( RecognizerConfigurationLoader, RecognizerListLoader, ) +# The component root, i.e. the directory that contains the ``presidio_analyzer`` +# package. Some shipped entries carry a ``config_path`` that the recognizer +# resolves relative to the current working directory, so the load test below runs +# from here -- the same working directory CI uses -- rather than catching the +# resulting error. Catching it would turn a genuinely missing shipped file into a +# passing skip. +PACKAGE_ROOT = Path(__file__).resolve().parent.parent + +DEFAULT_CONF = PACKAGE_ROOT / "presidio_analyzer" / "conf" / "default_recognizers.yaml" + +# Parsed once at import. The shipped-configuration tests below are parametrized +# over every entry, so re-reading the file per invocation would parse it a few +# dozen times to retrieve the same keys. +DEFAULT_CONF_DATA = yaml.safe_load(DEFAULT_CONF.read_text(encoding="utf-8")) +GLOBAL_REGEX_FLAGS = DEFAULT_CONF_DATA["global_regex_flags"] + def create_mock_pattern_recognizer(lang, entity, name): return PatternRecognizer( @@ -509,16 +529,9 @@ def test_default_recognizers_yaml_country_code_matches_class(): against the YAML and the code drifting silently — the loader will refuse to load on mismatch. """ - conf_path = ( - Path(__file__).resolve().parent.parent - / "presidio_analyzer" - / "conf" - / "default_recognizers.yaml" - ) - data = yaml.safe_load(conf_path.read_text()) declared = [ r - for r in data.get("recognizers", []) + for r in DEFAULT_CONF_DATA.get("recognizers", []) if isinstance(r, dict) and "country_code" in r ] assert declared, "expected at least one country_code: entry in YAML" @@ -571,3 +584,243 @@ def test_yaml_country_code_blank_value_raises(): recognizer_cls=UsSsnRecognizer, recognizer_name="UsSsnRecognizer", ) + + +# --------------------------------------------------------------------------- +# Contract between the loader and the predefined recognizers it builds +# +# ``RecognizerListLoader`` builds predefined recognizers from YAML by passing the +# entry's keys as constructor kwargs. That makes the constructor signature part +# of a contract which nothing else enforces: a recognizer can satisfy every one +# of its own unit tests -- which instantiate it directly -- and still be +# impossible to load from a registry configuration. +# +# The gap is specifically in the *disabled* entries. Most of what +# ``default_recognizers.yaml`` ships is ``enabled: false``, and no other test +# constructs those, so a broken constructor stays invisible until a user flips +# the switch. The tests below construct every one of them. +# --------------------------------------------------------------------------- + +# Kwargs ``RecognizerListLoader`` passes to every predefined recognizer it +# builds: ``name`` comes from the YAML entry (or its ``class_name`` alias) and +# ``supported_language`` from the resolved per-language configuration. +LOADER_KWARGS = ("name", "supported_language") + +# Entries that cannot load from their shipped configuration even with every +# dependency installed, so the load test below cannot cover them. +# +# ``HuggingFaceNerRecognizer``: ``EntityRecognizer.__init__`` calls ``load()`` +# unconditionally and ``load()`` requires ``model_name``, which the shipped +# entry does not supply -- it raises ValueError once ``transformers`` and +# ``torch`` are present. That is a pre-existing defect in the entry, not +# something this contract can assert away, and adding ``model_name`` here would +# make the test download a model. It stays covered by the resolve test. +NOT_LOADABLE_FROM_SHIPPED_ENTRY = {"HuggingFaceNerRecognizer"} + +# Entries gated behind an optional dependency, for which refusing to load with an +# actionable ImportError is the intended behavior. The skip is scoped to these +# names rather than to the exception type, so an ImportError from any other entry +# stays a failure instead of a green skip. +OPTIONAL_DEPENDENCY_ENTRIES = {"BasicLangExtractRecognizer"} + + +def _pattern_recognizer_classes() -> Dict[str, type]: + """Predefined ``PatternRecognizer`` subclasses, which the YAML loader builds. + + Non-pattern recognizers (NER/LLM/remote wrappers) are excluded: several are + not registrable from ``default_recognizers.yaml`` and some deliberately fix + their own display name. + """ + classes = {} + for attr in dir(predefined): + obj = getattr(predefined, attr) + if not isinstance(obj, type) or not issubclass(obj, EntityRecognizer): + continue + if obj in (EntityRecognizer, PatternRecognizer): + continue + if issubclass(obj, PatternRecognizer): + classes[attr] = obj + return classes + + +def _yaml_entries() -> List[Dict]: + """Normalize the shipped recognizer list to dict entries.""" + entries = [] + for entry in DEFAULT_CONF_DATA["recognizers"]: + entries.append({"name": entry} if isinstance(entry, str) else dict(entry)) + return entries + + +def _entry_languages(entry: Dict) -> List[str]: + """Languages an entry declares, in either supported YAML shape.""" + languages = entry.get("supported_languages") + if not languages: + return ["en"] + if isinstance(languages[0], str): + return list(languages) + return [item["language"] for item in languages] + + +def _entry_id(entry: Dict) -> str: + return entry.get("class_name") or entry["name"] + + +PATTERN_CLASSES = _pattern_recognizer_classes() +YAML_ENTRIES = _yaml_entries() +LOADABLE_YAML_ENTRIES = [ + entry + for entry in YAML_ENTRIES + if _entry_id(entry) not in NOT_LOADABLE_FROM_SHIPPED_ENTRY +] + + +def test_default_conf_has_entries(): + """Guard the fixtures themselves: an empty parse would pass everything.""" + assert PATTERN_CLASSES, "no predefined PatternRecognizer subclasses found" + assert YAML_ENTRIES, "no recognizers parsed from default_recognizers.yaml" + + +@pytest.mark.parametrize( + "entry_id", + sorted(NOT_LOADABLE_FROM_SHIPPED_ENTRY | OPTIONAL_DEPENDENCY_ENTRIES), +) +def test_exclusion_names_a_real_entry(entry_id): + """An exclusion must still match a shipped entry. + + Keeps the two lists above from rotting: if an entry is renamed, removed, or + fixed, the stale exclusion fails here instead of silently narrowing + coverage. + """ + assert entry_id in {_entry_id(entry) for entry in YAML_ENTRIES} + + +CONFIG_PATH_ENTRIES = [entry for entry in YAML_ENTRIES if entry.get("config_path")] + + +@pytest.mark.parametrize("entry", CONFIG_PATH_ENTRIES, ids=_entry_id) +def test_yaml_entry_config_path_points_at_a_shipped_file(entry): + """A ``config_path`` in a shipped entry must point at a file that ships. + + Asserted directly rather than left to the load test, which skips the entry + when its optional dependency is absent. A deleted or renamed config file is + a regression that must fail even in an environment that cannot construct the + recognizer at all. + """ + config_path = Path(entry["config_path"]) + resolved = config_path if config_path.is_absolute() else PACKAGE_ROOT / config_path + assert resolved.is_file(), ( + f"{_entry_id(entry)} declares config_path {entry['config_path']!r}, " + f"which does not resolve to a file ({resolved})" + ) + + +@pytest.mark.parametrize("class_name", sorted(PATTERN_CLASSES)) +def test_pattern_recognizer_accepts_loader_kwargs(class_name): + """Constructor must accept every kwarg the YAML loader passes. + + Catches the defect before the recognizer reaches the YAML at all: a class + added without ``name`` passes its own unit tests, and only fails once + someone tries to register it. + """ + parameters = inspect.signature(PATTERN_CLASSES[class_name].__init__).parameters + if any(p.kind == inspect.Parameter.VAR_KEYWORD for p in parameters.values()): + pytest.skip( + f"{class_name}.__init__ takes **kwargs, so the signature cannot show " + f"which kwargs it honors. Reported as a skip rather than a silent " + f"pass so the gap in coverage stays visible." + ) + missing = [kwarg for kwarg in LOADER_KWARGS if kwarg not in parameters] + assert not missing, ( + f"{class_name}.__init__ does not accept {missing}, which " + f"RecognizerListLoader passes to every predefined recognizer. " + f"Loading it from a registry YAML raises TypeError." + ) + + +@pytest.mark.parametrize("entry", YAML_ENTRIES, ids=_entry_id) +def test_yaml_entry_class_resolves(entry): + """Every shipped entry must name a real recognizer class. + + The resolved object is asserted to be a recognizer class rather than left to + the lookup raising, so an entry that resolves to some unrelated module + attribute fails here instead of downstream. + """ + recognizer_cls = RecognizerListLoader.get_existing_recognizer_cls( + recognizer_name=_entry_id(entry) + ) + assert isinstance(recognizer_cls, type) and issubclass( + recognizer_cls, EntityRecognizer + ), f"{_entry_id(entry)} resolves to {recognizer_cls!r}, not a recognizer class" + + +@pytest.mark.parametrize("entry", LOADABLE_YAML_ENTRIES, ids=_entry_id) +def test_yaml_entry_loads_when_enabled(entry, monkeypatch): + """Every shipped entry must load once ``enabled`` is true. + + ``enabled: false`` is an opt-in switch, not a disclaimer -- an entry that + cannot be turned on should not be listed. + + Driven through ``RecognizerRegistryProvider`` rather than + ``RecognizerListLoader.get`` directly, so that the configuration validation + a real user's entry passes through is covered too: an entry that the loader + could build but the validator rejects is just as unusable. + + Runs from ``PACKAGE_ROOT`` so that a ``config_path`` the recognizer resolves + against the working directory behaves as it does in CI. A FileNotFoundError + is therefore a real missing shipped file and is left to fail. + """ + entry_id = _entry_id(entry) + languages = _entry_languages(entry) + entry = dict(entry, enabled=True) + monkeypatch.chdir(PACKAGE_ROOT) + configuration = { + "global_regex_flags": GLOBAL_REGEX_FLAGS, + "supported_languages": languages, + "recognizers": [entry], + } + + try: + registry = RecognizerRegistryProvider( + registry_configuration=configuration + ).create_recognizer_registry() + except ImportError as exc: + if entry_id not in OPTIONAL_DEPENDENCY_ENTRIES: + raise + pytest.skip(f"{entry_id} needs an optional dependency: {exc}") + + # Asserted on the class rather than on ``registry.recognizers`` being + # non-empty: the loader drops a recognizer whose language the registry does + # not support with a log warning and no exception, so a merely non-empty + # registry would not prove that *this* entry is what loaded. + loaded = [type(r).__name__ for r in registry.recognizers] + assert entry_id in loaded, ( + f"{entry_id} is listed in default_recognizers.yaml but loaded " + f"nothing for languages {languages} (registry holds {loaded})" + ) + + +def test_yaml_entry_can_be_renamed_via_class_name(): + """``class_name`` + ``name`` must give the instance the configured name. + + This is the documented reason the loader passes ``name`` at all (see + ``RecognizerListLoader.get_recognizer_name``), so it is the behavior that + makes the kwarg contract above load-bearing rather than incidental. + """ + configuration = { + "global_regex_flags": 26, + "supported_languages": ["en"], + "recognizers": [ + { + "class_name": "UsSsnRecognizer", + "name": "MyRenamedSsnRecognizer", + "supported_languages": ["en"], + "type": "predefined", + "country_code": "us", + } + ], + } + registry = RecognizerRegistryProvider( + registry_configuration=configuration + ).create_recognizer_registry() + + assert [r.name for r in registry.recognizers] == ["MyRenamedSsnRecognizer"] From 7304d492523f8c77891a07c0c66ba86b48a2b1b2 Mon Sep 17 00:00:00 2001 From: Yonghye Kwon Date: Wed, 5 Aug 2026 11:26:16 +0900 Subject: [PATCH 5/5] test(analyzer): check loader kwargs on YAML-listed non-pattern recognizers too Follow-up to review feedback on #2170 suggesting a signature-level check driven from default_recognizers.yaml. The signature sweep was package-driven and scoped to PatternRecognizer subclasses, which left five shipped entries unreached: PhoneRecognizer, the two Za* ones, and the NER/LLM wrappers. Three of those the load test already constructs outright, but HuggingFaceNerRecognizer is excluded from it (its shipped entry supplies no model_name), so nothing in the suite touched its constructor at all. Parametrize over the union of both sources instead. Neither subsumes the other: the package sweep reaches a class before it is listed anywhere, which is how KrPassportRecognizer failed -- it could not be added to the yaml at all, so a yaml-driven check could never have named it -- while the yaml sweep reaches a listed class the package sweep skips by base class. Renamed to test_recognizer_accepts_loader_kwargs, since it is no longer pattern-only. Adds 5 params: 3 assertions and 2 skips, the skips being the **kwargs constructors whose signature cannot show which kwargs they honor. HuggingFaceNerRecognizer now surfaces as a named skip explaining why it is uncovered, rather than being silently absent. 297 passed, 3 skipped (was 294 / 1). Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/test_recognizers_loader_utils.py | 68 ++++++++++++++++--- 1 file changed, 60 insertions(+), 8 deletions(-) diff --git a/presidio-analyzer/tests/test_recognizers_loader_utils.py b/presidio-analyzer/tests/test_recognizers_loader_utils.py index bb222aa229..987b5c04d3 100644 --- a/presidio-analyzer/tests/test_recognizers_loader_utils.py +++ b/presidio-analyzer/tests/test_recognizers_loader_utils.py @@ -627,9 +627,16 @@ def test_yaml_country_code_blank_value_raises(): def _pattern_recognizer_classes() -> Dict[str, type]: """Predefined ``PatternRecognizer`` subclasses, which the YAML loader builds. - Non-pattern recognizers (NER/LLM/remote wrappers) are excluded: several are - not registrable from ``default_recognizers.yaml`` and some deliberately fix - their own display name. + Swept from the package rather than from the YAML, so that a class is checked + *before* it is listed anywhere. That is the direction the defect actually + travelled: ``KrPassportRecognizer`` was added in #1814 without ``name`` and + consequently could not be added to ``default_recognizers.yaml`` at all, so no + YAML-driven check could ever have named it. + + Non-pattern recognizers (NER/LLM/remote wrappers) are not swept here: several + are not registrable from ``default_recognizers.yaml`` and some deliberately + fix their own display name. The ones that *are* listed come back in via + ``_yaml_listed_classes`` below. """ classes = {} for attr in dir(predefined): @@ -674,10 +681,52 @@ def _entry_id(entry: Dict) -> str: ] +def _yaml_listed_classes() -> Dict[str, type]: + """Classes named by a shipped entry, resolved the way the loader resolves them. + + Adds the entries the package sweep skips because they are not + ``PatternRecognizer`` subclasses -- ``PhoneRecognizer``, the two ``Za*`` + ones, and the NER/LLM wrappers. Being listed is what makes them fair game: + the loader passes the kwargs to whatever the YAML names, whatever its base + class. + + Most of these are covered more strongly by the load test below, which + constructs them outright. The one this reaches that nothing else does is an + entry in ``NOT_LOADABLE_FROM_SHIPPED_ENTRY``: excluded from the load test and + not a ``PatternRecognizer``, its constructor would otherwise go unchecked. + + An entry that does not resolve is dropped rather than raised on, so a bad + entry is reported by ``test_yaml_entry_class_resolves`` as one named failure + instead of breaking collection for this whole module. + """ + classes = {} + for entry in YAML_ENTRIES: + entry_id = _entry_id(entry) + try: + cls = RecognizerListLoader.get_existing_recognizer_cls( + recognizer_name=entry_id + ) + except Exception: # noqa: BLE001 - reported by the resolve test + continue + if isinstance(cls, type) and issubclass(cls, EntityRecognizer): + classes[entry_id] = cls + return classes + + +# Every class the loader may be asked to build: swept from the package (catches a +# class before it reaches the YAML) and from the shipped YAML (catches a listed +# class the package sweep skips). Neither source subsumes the other. +LOADER_BUILT_CLASSES = {**PATTERN_CLASSES, **_yaml_listed_classes()} + + def test_default_conf_has_entries(): """Guard the fixtures themselves: an empty parse would pass everything.""" assert PATTERN_CLASSES, "no predefined PatternRecognizer subclasses found" assert YAML_ENTRIES, "no recognizers parsed from default_recognizers.yaml" + assert LOADER_BUILT_CLASSES.keys() >= PATTERN_CLASSES.keys(), ( + "the YAML-listed classes did not resolve, so the union collapsed to less " + "than the package sweep alone" + ) @pytest.mark.parametrize( @@ -714,15 +763,18 @@ def test_yaml_entry_config_path_points_at_a_shipped_file(entry): ) -@pytest.mark.parametrize("class_name", sorted(PATTERN_CLASSES)) -def test_pattern_recognizer_accepts_loader_kwargs(class_name): +@pytest.mark.parametrize("class_name", sorted(LOADER_BUILT_CLASSES)) +def test_recognizer_accepts_loader_kwargs(class_name): """Constructor must accept every kwarg the YAML loader passes. - Catches the defect before the recognizer reaches the YAML at all: a class - added without ``name`` passes its own unit tests, and only fails once + Signature-level on purpose, so it holds for recognizers that cannot be + constructed in a test at all -- an ML entry needing configuration, or one + whose dependencies are absent -- which the load test below has to exclude or + skip. It also catches the defect before the recognizer reaches the YAML: a + class added without ``name`` passes its own unit tests and only fails once someone tries to register it. """ - parameters = inspect.signature(PATTERN_CLASSES[class_name].__init__).parameters + parameters = inspect.signature(LOADER_BUILT_CLASSES[class_name].__init__).parameters if any(p.kind == inspect.Parameter.VAR_KEYWORD for p in parameters.values()): pytest.skip( f"{class_name}.__init__ takes **kwargs, so the signature cannot show "