🐛 fix: handle PEP 604 union syntax in list type detection (#139)#146
🐛 fix: handle PEP 604 union syntax in list type detection (#139)#146vawsgit wants to merge 3 commits into
Conversation
Fields declared with PEP 604 syntax (`list[X] | None`) produce a `types.UnionType` at runtime, which has no `__origin__` and is not recognized by `get_origin(...) is Union`. As a result, the type-detection methods failed to identify these as list fields, routing empty-list comparisons (`[]` vs `[]`) through the primitive comparator and yielding `match=False, score=0.0` instead of a true negative. ✨ Add `types.UnionType` handling to 5 type-detection methods: - `_is_list_field` (structured_model.py) - `_is_list_of_structured_model_type` (structured_model.py) - `is_structured_field_type` (configuration_helper.py) - `_is_list_structured_model` (configuration_helper.py) - `_extract_structured_class_from_list` (configuration_helper.py) ✅ Add `test_pep604_union_type_handling.py` with 24 tests covering type detection, empty/non-empty nested list comparison, primitive lists, and a parametrized validation suite guarding both PEP 604 and typing.Optional. 🧪 Full suite: 1100 passed, 2 skipped, 0 failures.
Security Scan ResultsASH Security Scan Report
Scan Metadata
SummaryScanner ResultsThe table below shows findings by scanner, with status based on severity thresholds and dependencies:
Top 7 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 19 actionable findingsFinding 1: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 2: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 3: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 4: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 5: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 6: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 7: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 8: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 9: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 10: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 11: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 12: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 13: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 14: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 15: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 16: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 17: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 18: yaml.github-actions.security.github-actions-mutable-action-tag.github-actions-mutable-action-tag
Description: Code Snippet: Finding 19: package_managers.uv.uv-missing-dependency-cooldown.uv-missing-dependency-cooldown
Description: Code Snippet: Report generated by Automated Security Helper (ASH) at 2026-07-16T22:11:53+00:00 |
adiadd
left a comment
There was a problem hiding this comment.
appreciate this vincil! just a couple things:
blocking:
- the PEP 604 branch in
is_structured_field_typedoes more than parity: its direct-model arm silently changes the default comparator and threshold forModel | Nonefields, and the behavior reverts on python 3.14. details and two fix options onconfiguration_helper.py:180-181.
non-blocking:
-
the five duplicated union blocks could collapse to a two-token change (
get_origin(annotation) in (Union, types.UnionType)) in 4 of the 5 methods, which would also make the blocking issue impossible by construction. seeconfiguration_helper.py:168. -
__init_subclass__validation now raises at import time for PEP 604 spellings it used to silently accept, correct but worth a PR-description note and a test (structured_model.py:281). -
test gaps:
Nonevs[]/NonevsNonefor the PEP 604 spelling, and afrom_jsonround-trip so the two branches only unit tests reach get integration coverage (comments inline). -
or arg is listin_is_list_fieldis another non-parity spot and untested (structured_model.py:734), and_is_optional_structured_model/_extract_structured_class_from_optionalin the same file are still PEP-604-blind, fine out of scope but worth a follow-up issue (configuration_helper.py:417,447).
nits, none blocking: style drift between the five new blocks (some hoist none_type = type(None), some inline it), the pep604 arm of TestIssue139Validation mostly duplicates the earlier tests, a direct per-field match=True assertion on items would pin the issue's exact symptom, and a one-line docs note on supported annotation syntax would help. also worth filing follow-ups beyond this PR: _unwrap_optional (structured_model.py:1384) is PEP-604-blind so schema export silently emits {"type": "string"} for these fields, data_extractors.py:205 misses both spellings in HTML reports, and field_helper.py:48 has a dead duplicate is_structured_field_type with no union handling at all.
appreciate it!
…ator parity Address PR #146 review (adiadd). Collapse the five near-identical typing.Union / types.UnionType branches into a single `get_origin(annotation) in (Union, types.UnionType)` check across all five type-detection methods. get_origin/get_args normalize both union spellings identically on 3.12, so the branches were pure duplication (~88 lines). Blocking fix: is_structured_field_type had a divergent PEP 604 arm that returned True for a direct `Model | None` field, silently switching its default comparator from LevenshteinComparator@0.5 to StructuredModelComparator@0.9 (flipping match decisions for scores in [0.5, 0.9)). The unified branch only recognizes list-wrapped models, so `Model | None` now has parity with `Optional[Model]`. The collapse also makes the divergence structurally impossible and is correct on Python 3.14, where the two union types unify. Also drop the `or arg is list` asymmetry in _is_list_field so `list | None` matches the bare-`list` / `Optional[list]` behavior. Tests (+9): direct Model|None vs Optional[Model] parity, None-vs-[] and None-vs-None contract, per-field true-negative assertion for [] vs [], from_json round-trip coverage for the list-detection methods, and __init_subclass__ ValueError parity for the PEP 604 spelling.
|
Thanks for the thorough review @adiadd — you were right on every point, and taking the refactor you suggested made most of them fall out for free. Pushed b9f9473. Blocking — resolved via the refactorCollapsed all five Non-blocking — addressed
Full suite: 1109 passed, 2 skipped, 0 failures. Follow-ups — filed as separate issuesAll four out-of-scope spots you flagged are now tracked:
|
Fields declared with PEP 604 syntax (
list[X] | None) produce atypes.UnionTypeat runtime, which has no__origin__and is not recognized byget_origin(...) is Union. As a result, the type-detection methods failed to identify these as list fields, routing empty-list comparisons ([]vs[]) through the primitive comparator and yieldingmatch=False, score=0.0instead of a true negative.✨ Add
types.UnionTypehandling to 5 type-detection methods:_is_list_field(structured_model.py)_is_list_of_structured_model_type(structured_model.py)is_structured_field_type(configuration_helper.py)_is_list_structured_model(configuration_helper.py)_extract_structured_class_from_list(configuration_helper.py)✅ Add
test_pep604_union_type_handling.pywith 24 tests covering type detection, empty/non-empty nested list comparison, primitive lists, and a parametrized validation suite guarding both PEP 604 and typing.Optional.🧪 Full suite: 1100 passed, 2 skipped, 0 failures.
Issue #, if available:
#139
Description of changes:
🔍 Root Cause
Python 3.10+ PEP 604 syntax (
X | Y) produces atypes.UnionTypeat runtime, which behaves differently fromtyping.Union:The type-detection methods only checked
get_origin(...) is Unionandhasattr(field_type, "__origin__"), so PEP 604 unions silently fell through toreturn False. The comparison logic itself was correct — the bug was purely in type detection.✨ Changes
Added
types.UnionTypehandling to 5 type-detection methods:structured_model.py_is_list_fieldstructured_model.py_is_list_of_structured_model_typeconfiguration_helper.pyis_structured_field_typeconfiguration_helper.py_is_list_structured_modelconfiguration_helper.py_extract_structured_class_from_listEach method now unwraps
types.UnionTypeargs (skippingNoneType) using the same logic already applied totyping.Union.✅ Tests
New file
test_pep604_union_type_handling.py— 24 tests:typing.Optional[List[X]]behavior unchangedTestIssue139Validation) — runs every assertion against both PEP 604 andtyping.Optionalstyles in one command:🧪 Test Results
No regressions — all existing tests continue to pass because they use
typing.Union/typing.Optionalsyntax.📌 Notes
ComparableFieldinterface changestypesis stdlib; project requires Python>=3.12wheretypes.UnionTypeis guaranteed)Closes #139
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
🔄 Update (addressing review #146)
Per @adiadd's review, the five duplicated union branches were collapsed to a single
get_origin(annotation) in (Union, types.UnionType)check across all five type-detection methods. This fixes the blocking issue by construction:is_structured_field_typeno longer has a divergent direct-Model | Nonearm, soModel | Nonekeeps parity withOptional[Model](both route throughLevenshteinComparator), and behavior no longer reverts on Python 3.14.Fields spelled
list[Model] | None = ComparableField(threshold=...)(PEP 604) now raiseValueErrorat class-definition time — the same guard that has always applied toOptional[List[Model]]. Hungarian matching uses each element class'smatch_threshold, so a per-fieldthresholdon a list-of-models field is rejected. Previously the PEP 604 spelling silently bypassed this validation.📌 Supported annotation syntax
List/optional detection now handles
list[X],List[X],Optional[List[X]], and PEP 604list[X] | Noneuniformly.🔭 Tracked follow-ups (out of scope)
_is_optional_structured_model,_extract_structured_class_from_optional,_unwrap_optional,data_extractors.py:205, and the deadfield_helper.py:48duplicate remain PEP-604-blind — to be filed as separate issues.