Repository navigation
Fixes #34390: Recognize identifier variants and validate NRIC/FIN check letters - #34672
stout-yeoman wants to merge 13 commits into
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
stout-yeoman
added
the
safe to test
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
Code Review ✅ Approved 2 closed / 2 findings🔴 High risk · Changing NRIC/FIN and other PII recognition and persisted tags could leave identity-bearing columns unflagged or incorrectly classify ordinary fields. Adds identifier variant recognition for lowercase IBAN, NIF, NIE and UEN, hyphenated ABN/ACN, and attached Italian VAT prefixes, with NRIC/FIN check letter validation across all families. Legacy ACN/ABN/VAT/UEN stay Sensitive when NonSensitive is not a candidate, and compact IBAN followed by a number in prose is no longer detected. No open issues found. ✅ 2 closed✅ Bug: Legacy ACN/ABN/VAT/UEN stay Sensitive when NonSensitive is not a candidate
✅ Edge Case: Compact IBAN followed by a number in prose is no longer detected
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
This branch has not been deployed
3 participants
Describe your changes:
Fixes #34390
Recognize lowercase IBAN, NIF, NIE and UEN values, hyphenated ABN/ACN values, and attached Italian VAT prefixes. Reject incorrect NRIC/FIN check letters for S/T/F/G/M families and unsupported prefixes matched by Presidio's weak pattern.
Valid NRIC/FIN checksums retain the 0.5 pattern score. Lowercase variants and hyphenated ABN/ACN forms retain pattern scores and need matching context to reach the default tagging threshold. Canonical forms retain upstream checksum scoring. Attached VAT, such as
IT12345678903, validates at full confidence. Checksums validate representation, not actual issuance.Type of change:
High-level design:
Decorated factories and small subclasses in
presidio_utils.pyapply format normalization/checksum checks;patterns.pysupplies bounded identifier patterns. Presidio retains responsibility for IBAN spans/trailing prose. Existing configured entity names, languages and contexts remain supported, with no schema or migration changes.Attribution uses the configured recognizer name and stored recognizer ID, following the upstream factory contract. The database test now asserts
SgFinRecognizerrather than the implementation subclass name. Negative configured-path tests pair each rejected value with a valid control and inspect actual TagAnalyzer evidence/original spans, so an obsolete or disconnected recognizer cannot make them pass vacuously. Attached VAT is exercised through TagProcessor with both neutral and identifying columns and through the real Italian database fixture.Tests:
Use cases covered
Valid/invalid checksums, all NRIC/FIN families, canonical and new formats, original spans and punctuation, Unicode lookalikes, larger-token boundaries, configured language/context, context-dependent scores, neutral-column controls, attached VAT at score 1.0, and persisted recognizer identity/pattern evidence.
Unit tests
At
0cf68d9f5130db719ee1e859cd1b239f6bfa8802:cd ingestion ../env/bin/python -m pytest tests/unit/pii tests/unit/metadata/pii --cov=metadata.pii.algorithms.presidio_utils --cov=metadata.pii.algorithms.patterns --cov-branch --cov-report=term-missing -q516 passed; 98% utility and 100% pattern coverage including branches. Updated
tests/unit/pii/algorithms/test_presidio_utils.pyandtests/unit/metadata/pii/test_tag_processor_integration.py; the factory suite also ran. Focused hardening checks: 11 passed (nine paired identifier cases, two attached VAT contexts). A deliberate valid-NRIC negative-oracle probe demonstrates the corrected assertion fails when invalid attribution is introduced. Applicable Ruff/format, pre-commit and whitespace checks passed; source type checks report 0 errors/0 warnings (the final delta is test-only). Independent final review found no actionable issues.Earlier runs: 442 PII tests on the original implementation, 161 focused utility/factory tests on the attribution correction, and 514 full related tests at
a96a9afbcf; the 516-test run above supersedes their counts/coverage.Backend integration tests
Not applicable: no Java, REST endpoint, schema, migration, seed or pom changes.
python3 .github/scripts/plan_local_java_tests.py --check-branchpassed; no affected Java tests.Ingestion integration tests
Updated
ingestion/tests/integration/auto_classification/databases/{init.sql,test_tag_processor.py}. Ran the complete affected database file against real PostgreSQL/OpenMetadata at0cf68d9f5130: 4 passed, covering default classification plus English, Spanish and Italian identifier workflows. Used normal repository fixtures/credentials with only the local endpoint adapted to port 18585. Verified persisted configured recognizer ID/name/score/pattern evidence, typo exclusion and neutral-column controls. The Italian fixture uses attachedIT12345678903.The baseline reproduced the stale subclass-name assertion: 1 failed, 3 deselected; both the attribution correction and final attached-VAT fixture reruns passed. No packaging/model code changes or clean-wheel certification is claimed; the separate model-provisioning check is wheel-gated.
Playwright (UI) tests
Shared validation uses local combined commit
d84bd7b9204de848334bce46f7e033ccefb6f804: the production union of exact published card head512b065669and identifier head0cf68d9f51, plus32 local preservation/control tests. UI sources and lockfiles are unchanged by either PR. A matching freshly built backend, production UI bundle, real managed Airflow runner and isolated MySQL/Metabase/Kafka/schema-registry fixtures resolve the earlier missing-team-type/runner prerequisites.The current combined impact planner selected 37 specs /616 tests. Full original execution against the Vite development UI completed 603 passed,13 failed,0 skipped. Original failures and traces are retained. Ten failures were traced to shared global settings interference/precondition conflicts (domain search, generic entity domain/glossary helpers and the logical test suite); three bulk-edit variants stalled before export in development mode.
Unchanged failed cases were rerun sequentially against the production UI with their normal settings preconditions. All13 original failing test IDs/projects now pass: Entity9/9 (including one extra control), Logical1/1, Domain3/3, Bulk3/3, plus setup2/2. Production bulk success supports the development lifecycle explanation. This is a full original run plus controlled reruns, not a claim that one uninterrupted616-case run was green. No assertion or product source was weakened. All six AutoPilot connectors and the real Incident workflow passed in the original run. A separate local tooling repair isolates global-setting writers and restores settings; it is outside both PII PRs.
Additional shared regression validation
TestCaseEvaluationScope; no full current-schema downstream certification is claimed.Manual testing performed
metadata install-classification-models, and confirmed imports/CLI use the isolated changed checkout.metadata ingest -c ...; ranmetadata classify -c ...with the shipped TagProcessor for English, Spanish and Italian tables.a96a9afbcf; production code is identical at final0cf68d9f5130(only tests/fixture changed).UI screen recording:
Not applicable: backend ingestion bug fixes, with no UI feature or UI source changes. The normal ingestion/classification CLI was exercised as described above.
Checklist:
Fixes <issue-number>: <short explanation>and the issue is linked above.Merge coordination
Coordinate with #34387 (issue #34347): retain its ACN zero-check-digit correction together with this PR's hyphenated form/context-dependent scoring. That validator correction remains separate.
This PR shares recognizer registration and database fixtures with #34674. Merge sequentially, reconcile/rebase shared files and rerun the combined fixture and configured TagProcessor checks.