Conversation
Add an optional capture_group to Pattern (a group number or name). When it is set, PatternRecognizer reports the span of that group instead of the whole match, so a pattern such as "password:\s*(\S+)" can detect only the value. Matches in which the group does not participate are skipped, and validate_result/invalidate_result receive the group text. capture_group is checked when the Pattern is built: bool, negative, out of range and unknown group names raise ValueError. Pattern.to_dict emits the key only when it is set, so serialised output is unchanged for existing patterns. The YAML CustomRecognizerConfig validates it at parse time, and ad-hoc REST recognizers accept it through from_dict. If regex flags such as re.VERBOSE remove the group at analysis time, the pattern is skipped with a warning that names the pattern only. Behaviour change: none when capture_group is not set. Patterns that set it were rejected before (TypeError in Pattern.__init__, HTTP 400 for ad-hoc recognizers). Fixes data-privacy-stack#1120
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate warning-handling issue remains in pattern_recognizer.py.
Pull request overview
Adds optional regex capture-group reporting so recognizers can return a subgroup instead of the full match.
Changes:
- Adds validation and serialization for numeric or named
capture_group. - Applies selected spans across Python, YAML, REST, and analyzer flows.
- Adds tests and documentation while preserving whole-match behavior when unset.
- Moderate finding (1 vote): Flags-incompatible patterns produce no warning when there are no matches; validation should occur before iterating matches.
File summaries
| File | Description |
|---|---|
presidio-analyzer/tests/test_yaml_recognizer_models.py |
Tests YAML capture-group validation. |
presidio-analyzer/tests/test_recognizer_registry_provider.py |
Tests provider integration. |
presidio-analyzer/tests/test_pattern.py |
Tests validation and serialization. |
presidio-analyzer/tests/test_pattern_recognizer.py |
Tests matching behavior and flags. |
presidio-analyzer/tests/test_context_support.py |
Tests context interaction. |
presidio-analyzer/tests/test_analyzer_request.py |
Tests ad-hoc recognizers. |
presidio-analyzer/presidio_analyzer/pattern.py |
Adds capture-group support. |
presidio-analyzer/presidio_analyzer/pattern_recognizer.py |
Reports selected group spans and handles flags. |
presidio-analyzer/presidio_analyzer/input_validation/yaml_recognizer_models.py |
Validates configured groups. |
e2e-tests/tests/test_api_analyzer.py |
Verifies REST behavior. |
docs/api-docs/api-docs.yml |
Documents the API schema. |
docs/analyzer/recognizer_registry_provider.md |
Documents YAML configuration. |
docs/analyzer/adding_recognizers.md |
Documents capture-group usage. |
Review details
Suppressed comments (1)
presidio-analyzer/presidio_analyzer/pattern_recognizer.py:232
- Because the invalid-group check is inside
for match in matches, a flags-incompatible pattern emits no warning when the whole regex has no matches. The documentation promises a warning for this configuration and once-per-call behavior, so inspectcompiled_regex.groups/groupindexbefore iterating and skip the pattern there; otherwise the misconfiguration remains silent on inputs without a candidate.
group = 0 if pattern.capture_group is None else pattern.capture_group
for match in matches:
try:
start, end = match.span(group)
except IndexError:
- Files reviewed: 13/13 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The check for a capture group removed by the regex flags in use ran inside the match loop, so a misconfigured pattern only logged its warning when the text happened to contain a match of the whole regex. Check the compiled regex's groups before iterating instead: the warning no longer depends on the input, and the match loop goes back to a plain match.span(group). The check only runs when capture_group is set, so patterns without it never touch the compiled regex's groups.
|
Good catch from the Copilot review (it ended up in the suppressed comments, so replying here): the check for a group removed by the regex flags lived inside Fixed in ff16851:
Verification: the 6 touched test modules 211 passed; full |
Change Description
Adds an optional
capture_grouptoPattern, so aPatternRecognizercan report one capture group instead of the whole regex match. For examplepassword:\s*(?P<value>\S+)withcapture_group="value"detectshunter2, notpassword: hunter2.Root cause.
PatternRecognizer.__analyze_patternsalways takesmatch.span(), so whatever a regex needs as an anchor ends up inside the entity. The only ways around it today are a variable-width lookbehind or a subclass with its own matching loop (which is what the reporter of #1120 ended up doing).What changes
Patterncapture_group: Optional[Union[int, str]] = None, a group number or name. Checked at construction against the compiled regex: bool, negative, out-of-range and unknown names raiseValueError. Messages never include matched textPattern.to_dictcapture_grouponly when it is set, so serialised output of existing patterns stays byte-identical (checked for all 108 predefined pattern recognizers)PatternRecognizermatch.span(capture_group). Matches where the group doesn't participate are dropped by the existing empty-match check, andvalidate_result/invalidate_resultreceive the group textCustomRecognizerConfig(YAML)capture_groupat parse time when it is present. YAML without it is validated exactly as beforefrom_dict, no loader changeapi-docs.ymlPattern schema, a "Detecting part of a regex match" section inadding_recognizers.md,recognizer_registry_provider.mdBehaviour changes
capture_groupis set:match.span(0)ismatch.span(), and an/analyzeresponse withreturn_decision_processis identical tomain.capture_groupused to fail (TypeErrorinPattern.__init__, HTTP 400 for ad-hoc recognizers) and now work.password:that is also a context word raises the score. Documented and covered by a test.re.VERBOSEturning#...into a comment), the pattern is skipped with a warning (pattern name and group only), once peranalyzecall and regardless of the input text. A group number can also shift under such flags, so the docs recommend named groups there.AnalysisExplanationis unchanged, since the score formula is unchanged.Design decisions
( ... ), including the zip example in the docs. Several groups → several patterns on the same regex.capture_grouprather thanuse_groupfrom the issue: I think it reads better in YAML and JSON. Happy to rename.regexmodule does support(?<=password:\s*)\S+, and a few predefined recognizers use exactly that. Still, a group keeps the regex portable and readable, is settable from YAML/REST without lookbehind tricks, fails early with a clear message, and hands the validation hooks only the value.Not in scope
IbanRecognizer) ignorecapture_group. Documented.Patterndoesn't see any of them at construction.capture_groupin an ad-hoc recognizer is 500, same as an invalidscoretoday (ValueError→ 500 inapp.py).Verification
In
presidio-analyzer, Python 3.12,uv sync --locked --all-extras --group devplusen_core_web_lg/en_core_web_sm:pytest tests/test_pattern.py tests/test_pattern_recognizer.py tests/test_yaml_recognizer_models.py tests/test_recognizer_registry_provider.py tests/test_analyzer_request.py tests/test_context_support.py: 211 passed. The same tests againstmain's sources: 31 failed.pytest --cov=presidio_analyzer): 3498 passed, 13 skipped (main: 3463 passed, 13 skipped; the difference is the 35 new tests).diff-cover coverage.xml --compare-branch=origin/main --fail-under=90: 41 changed lines, 100%.ruff checkwith ruff 0.9.2 as in CI: clean.tests/test_api_analyzer.py -k ad_hocagainst a local analyzer started with the Dockerfile's conf: 5 passed (the new test gets a 400 onmain). I didn't run the Docker e2e itself.api-docs.ymlpassesopenapi-spec-validator.The new tests cover spans for a number, a name,
0and unset; non-participating and empty groups; what the hooks receive; two patterns on one regex; the flags warning (logged once per call, no PII in the log); theto_dict/from_dictround trip; YAML throughRecognizerRegistryProvider; ad-hoc request dicts; and the context-word interaction.Issue reference
Fixes #1120
Checklist