Repository navigation
fix(hip): parameters=[(code, data)] loses values whose maker keyword differs from the data field name (#2352) - #2383
Conversation
JarryShaw
added
bug
|
Coverage: 91.58% (unit tier, Python 3.15,
Per-file detail: the |
|
Review: GOOD TO GO at Found 2 MORE real casualties of the same bug beyond the original 16, both fixed by this same change but currently unpinned: |
…differs from the data field name (#2352) Problem: HIP._make_hip_param's (code, value) branch rebuilds each parameter by expanding value as **kwargs into the maker. A parsed Data_Parameter is a Mapping, so it expands too -- but 18 parameters' maker keywords are spelled differently from the data model's own field names (suite_id vs suites, group_id vs group, reg_type vs reg_request/reg_response/reg_failed/reg_info, a nested locator key colliding with _make_locator's own first parameter, and others), so those values silently fell back to the maker's default instead of reaching it, or raised AttributeError outright. HIP_TRANSFORM rebuilt as 0241000000000000, losing its suite_id. Fix: a parsed Data_Parameter now goes to its maker positionally, through a new _make_hip_param_data helper shared with the OrderedMultiDict branch, which already called positionally and was already correct. The dict-form keyword API (parameters=[(code, {'suites': [...]})]) is untouched. Test: tests/protocols/internet/test_hip_param_data_rebuild_2352_unit.py round-trips 19 cases over 18 renamed parameters through parse -> rebuild via the (code, data) form -- 15 makers whose renaming is detected mechanically from their keyword-only parameters against the data model's own field annotations, plus ROUTE_DST, ROUTE_VIA and LOCATOR_SET, hand-listed because their renaming is nested inside a sub-model the mechanical check can't see. A HOST_ID case with a non-ECDSA algorithm is included alongside the existing ECDSA one, which happens to round-trip either way. 36 of its subtests plus one further test fail on the parent commit.
9305846 to
30fce1c
Compare
|
Amended All findings now addressed. |
|
Ready to merge. Review: GOOD TO GO (Sonnet) at |
JarryShaw
added
the
review: good-to-go
JarryShaw
removed
the
review: good-to-go
make testpasses, and a test case covers the changeWhat is the purpose of your pull request?
fix — bug fixDescription
Closes #2352.
HIP._make_hip_param's(code, value)branch rebuilds each parameter by expandingvalueas**kwargsinto the maker. A parsedData_Parameteris aMapping, so it expands too — but 18 parameters' maker keywords are spelled differently from the data model's own field names (suite_idvssuites,group_idvsgroup,reg_typevsreg_request/reg_response/reg_failed/reg_info, a nestedlocatorkey colliding with_make_locator's own first parameter, and others), so those values silently fell back to the maker's default instead of reaching it, or raisedAttributeErroroutright.HIP_TRANSFORMrebuilt as0241000000000000, losing itssuite_id— exactly the issue's repro.Data_Parameternow goes to its maker positionally, through a new_make_hip_param_datahelper shared with theOrderedMultiDictbranch, which already called positionally and was already correct. The dict-form keyword API (parameters=[(code, {'suites': [...]})]) is untouched.tests/protocols/internet/test_hip_param_data_rebuild_2352_unit.pyround-trips 19 cases over 18 renamed parameters through parse → rebuild via the(code, data)form — 15 makers whose renaming is detected mechanically from their keyword-only parameters against the data model's own field annotations, plusROUTE_DST,ROUTE_VIAandLOCATOR_SET, hand-listed because their renaming is nested inside a sub-model the mechanical check can't see. AHOST_IDcase with a non-ECDSA algorithm is included alongside the existing ECDSA one, which happens to round-trip either way. 36 of its subtests plus one further test fail on the parent commit.