Skip to content

fix(analyzer): remove a non-language code and a duplicate entry from default_recognizers.yaml - #2236

Open
heeoneie wants to merge 3 commits into
data-privacy-stack:mainfrom
heeoneie:fix/recognizer-config-language-and-duplicate
Open

fix(analyzer): remove a non-language code and a duplicate entry from default_recognizers.yaml#2236
heeoneie wants to merge 3 commits into
data-privacy-stack:mainfrom
heeoneie:fix/recognizer-config-language-and-duplicate

Conversation

@heeoneie

Copy link
Copy Markdown

Change Description

Two data problems in presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml, with a regression test for each.

1. kr is not a language code

The five Korean entries listed kr alongside ko under supported_languages. ko is the ISO 639-1 language code; kr is the ISO 3166-1 country code, which those same entries already carry in country_code. Korea is the only country in the file where the country code leaked into the language list — Canada, the closest analogue, correctly pairs country_code: ca with supported_languages: [en, fr].

RecognizerListLoader._get_recognizer_languages builds one recognizer per listed language, so what kr costs depends on how the registry is configured:

registry supported_languages before after
["ko"] 5 Korean recognizers, plus 5 "language is not supported" warnings 5 recognizers, no warnings
["en", "ko"] 10 Korean recognizers — every one registered twice 5

The second copy can never match anything: there is no spaCy kr model, so no NLP engine can serve that language while its regexes still run.

ConfigurationValidator.validate_language_codes does not catch this — kr is a well-formed two-letter code, just not the right one.

This also touches one existing test. #2170 parametrised test_loads_from_default_recognizers_yaml over ["ko", "kr"], following the convention already in the file (kr goes back to #1675). That made the kr case assert the defect rather than the contract, so this PR drops it and keeps ko.

2. UkPostcodeRecognizer was listed twice

#1858 added it; #1857 added it again and merged later, so the file carried two byte-identical blocks and the registry built two identical instances.

To be precise about severity: analyzer output is unaffected, because identical spans are merged downstream. The cost is a duplicate regex pass and a config file that misstates itself.

Issue reference

No existing issue — I searched and found none for either finding. Happy to open one first if you would rather have that order.

Verification

  • Both new tests fail on main and pass with the config fix:
    • duplicate recognizer entries: ['UkPostcodeRecognizer']
    • non-language codes in supported_languages: {'KrBrnRecognizer': ['kr'], 'KrRrnRecognizer': ['kr'], ...} — all five
  • Full presidio-analyzer suite, before and after: the set of failing and erroring tests is byte-identical, 36 items, all pre-existing and caused by optional dependencies missing in my environment (langextract, nlp_engine_provider, azure, slim_spacy). diff of the two FAILED/ERROR lists is empty.
  • The before/after table above was measured, not inferred: I built a registry from the shipped YAML with the Korean entries enabled and counted registry.recognizers for each language configuration.
  • ruff check and ruff format --check on both touched test files report exactly what they report on main (one pre-existing finding in test_recognizer_registry.py, seven in test_kr_passport_recognizer.py). My lines add none, and I left the pre-existing ones alone rather than widen the diff.

A note on the language guard

test_default_recognizers_yaml_declares_languages_not_countries keeps an explicit set of ISO 639-1 codes, so that adding a language becomes a deliberate edit rather than a side effect of a new entry. If you would rather not carry that list, I am glad to drop the test and keep the data fix, or reshape it however you prefer.

AI use

Claude Opus 5 assisted with the investigation, the edits, and running the checks above; each commit carries a Generated-by: trailer. Every claim here was verified against a local checkout before it was written.

Checklist

  • I have reviewed the contribution guidelines
  • I agree to follow this project's Code of Conduct
  • I confirm that I have the right to submit this contribution and that it does not knowingly contain proprietary or confidential code.
  • My code includes unit tests
  • All unit tests and lint checks pass locally — scoped exactly as described under Verification
  • My PR contains documentation updates / additions if required

…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: data-privacy-stack#1858 added it, and data-privacy-stack#1857 added it again and merged later. The registry built two identical instances.

Generated-by: Claude Opus 5
… 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
data-privacy-stack#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
@heeoneie heeoneie changed the title Fix/recognizer config language and duplicate fix(analyzer): remove a non-language code and a duplicate entry from default_recognizers.yaml Aug 28, 2026
@heeoneie

Copy link
Copy Markdown
Author

A correction to my own description — I did the archaeology after opening this, and the kr entries have more history than I gave them credit for.

kr was not a slip. #1742 (merged 2025-10-08) moved KrRrnRecognizer's class default from supported_language="kr" to "ko", and a second commit in that same PR deliberately added kr back to the YAML next to ko. Read as a backward-compatibility alias that is a reasonable call: a registry configured with kr — the only value that worked until then — would otherwise have gone quiet.

Where I think the argument still holds is that the alias was then copied onto recognizers that never had a kr default:

recognizer class default at introduction moved to ko legacy kr users possible
KrRrnRecognizer kr (2025-07-30) #1742, 2025-10-08 yes
KrPassportRecognizer kr (2025-12-25) #2170, 2026-08-05 yes
KrDriverLicenseRecognizer ko (2025-12-30) no
KrBrnRecognizer ko (2026-01-15) no
KrFrnRecognizer ko (2026-01-15) no

For the bottom three, ko is the only default the class has ever had, so no configuration can have depended on kr and removing it is pure cleanup. For KrRrn and KrPassport it is a real deprecation question instead — and KrPassport's default only moved three weeks ago.

So, how would you like this shaped?

  1. As it stands — drop kr everywhere, treating the alias as served out (close to a year for KrRrn).
  2. Drop it only from the three that never had a kr default, and leave KrRrn and KrPassport alone.
  3. Drop it everywhere, plus a line in the migration docs.

I am happy with any of them; 2 is the conservative one and I will push it if you prefer. The UkPostcodeRecognizer duplicate and both regression tests are unaffected either way — though under 2 the language guard would need an explicit allowance for those two entries, which is arguably a good place to record the deprecation.

Sorry for the incomplete first framing. #1742 made a deliberate choice and my description read as though nobody had considered it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant