diff --git a/presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml b/presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml index 4dc93f2bc4..2f38404418 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_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/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/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_kr_passport_recognizer.py b/presidio-analyzer/tests/test_kr_passport_recognizer.py index b6d7d93f35..a51914ee17 100644 --- a/presidio-analyzer/tests/test_kr_passport_recognizer.py +++ b/presidio-analyzer/tests/test_kr_passport_recognizer.py @@ -1,7 +1,15 @@ +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 +33,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 +41,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,8 +77,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 assert_result_within_score_range( @@ -104,7 +111,58 @@ 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. + + 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"] + + # 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_recognizers_loader_utils.py b/presidio-analyzer/tests/test_recognizers_loader_utils.py index 7a5bf2b7cd..987b5c04d3 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,295 @@ 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. + + 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): + 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 _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( + "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(LOADER_BUILT_CLASSES)) +def test_recognizer_accepts_loader_kwargs(class_name): + """Constructor must accept every kwarg the YAML loader passes. + + 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(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 " + 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"]