From b93f10a135c034c03a4864a2e01344222322d0ff Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 27 Oct 2026 12:33:58 -0400 Subject: [PATCH] fix(protocols): resolve a str enum name when its argument defaults to None (#2356) _make_index infers the enum namespace to search for a str name from the resolved argument's own default value. hopopt/ipv6_opts's seed_type and tid_type, and httpv1's status, all default to None rather than a member, so a str name ('SEEDID_16_BIT_UNSIGNED_INTEGER', 'CODE_404', ...) raised ProtocolNotImplemented instead of resolving -- the member and int forms of the same call already worked. Apply the #2280 fix (namespace=X if X is not None else ) at the 5 call sites: hopopt/ipv6_opts _make_opt_mpl and _make_opt_smf_dpd, httpv1._make_start_line. A wider scan found 12 more affected sites in hip.py, same symptom but a different shape (a loop variable over a list argument, not a bare None-default name) -- left out of this fix and filed separately as #2356's enums are imported only under TYPE_CHECKING there, so fixing them means promoting imports to runtime rather than a one-line change. Tests: the 5 fixed sites in test_make_index_str_unit.py, plus two stale-prose repairs in the same file that pinned the old broken behaviour's rationale rather than its actual cause. --- pcapkit/protocols/application/httpv1.py | 9 +- pcapkit/protocols/internet/hopopt.py | 18 +++- pcapkit/protocols/internet/ipv6_opts.py | 18 +++- tests/protocols/test_make_index_str_unit.py | 100 +++++++++++++++++++- 4 files changed, 132 insertions(+), 13 deletions(-) diff --git a/pcapkit/protocols/application/httpv1.py b/pcapkit/protocols/application/httpv1.py index 3af432d0fe..f1c48b70ec 100644 --- a/pcapkit/protocols/application/httpv1.py +++ b/pcapkit/protocols/application/httpv1.py @@ -348,8 +348,13 @@ def _make_start_line(self, http_version: 'str | bytes', *, header_line = b'%s %s HTTP/%s\r\n' % (meth, uri_val, version) elif method is None and status is not None: - status_code = self._make_index(status, status_default, namespace=status_namespace, - reversed=status_reversed, pack=False) + # NOTE: ``status`` defaults to :data:`None`, so ``_make_index`` has no + # enum default to infer the namespace of a :obj:`str` member name from; + # name it here rather than leave the name unresolvable (#2356). + status_code = self._make_index( + status, status_default, + namespace=status_namespace if status_namespace is not None else Enum_StatusCode, + reversed=status_reversed, pack=False) status_code_val = int(status_code) if message is None: diff --git a/pcapkit/protocols/internet/hopopt.py b/pcapkit/protocols/internet/hopopt.py index 99ee315a46..70d73ced73 100644 --- a/pcapkit/protocols/internet/hopopt.py +++ b/pcapkit/protocols/internet/hopopt.py @@ -1746,8 +1746,13 @@ def _make_opt_smf_dpd(self, code: 'Enum_Option', opt: 'Optional[Data_SMFIdentifi tid_len = len(tid_bytes) if tid_type is not None: - tid_kind = self._make_index(tid_type, tid_type_default, namespace=tid_type_namespace, - reversed=tid_type_reversed, pack=False) + # NOTE: ``tid_type`` defaults to :data:`None`, so ``_make_index`` has no + # enum default to infer the namespace of a :obj:`str` member name from; + # name it here rather than leave the name unresolvable (#2356). + tid_kind = self._make_index( + tid_type, tid_type_default, + namespace=tid_type_namespace if tid_type_namespace is not None else Enum_TaggerID, + reversed=tid_type_reversed, pack=False) elif tid_len == 0: tid_kind = Enum_TaggerID.NULL elif tid_len == 4: @@ -2056,8 +2061,13 @@ def _make_opt_mpl(self, code: 'Enum_Option', opt: 'Optional[Data_MPLOption]' = N seed = opt.seed_id if seed_type is not None: - kind = self._make_index(seed_type, seed_type_default, namespace=seed_type_namespace, - reversed=seed_type_reversed, pack=False) + # NOTE: ``seed_type`` defaults to :data:`None`, so ``_make_index`` has no + # enum default to infer the namespace of a :obj:`str` member name from; + # name it here rather than leave the name unresolvable (#2356). + kind = self._make_index( + seed_type, seed_type_default, + namespace=seed_type_namespace if seed_type_namespace is not None else Enum_SeedID, + reversed=seed_type_reversed, pack=False) elif seed is None or isinstance(seed, ipaddress.IPv6Address): kind = Enum_SeedID.IPV6_SOURCE_ADDRESS else: diff --git a/pcapkit/protocols/internet/ipv6_opts.py b/pcapkit/protocols/internet/ipv6_opts.py index 703d6798d0..0c51639ed4 100644 --- a/pcapkit/protocols/internet/ipv6_opts.py +++ b/pcapkit/protocols/internet/ipv6_opts.py @@ -1734,8 +1734,13 @@ def _make_opt_smf_dpd(self, code: 'Enum_Option', opt: 'Optional[Data_SMFIdentifi tid_len = len(tid_bytes) if tid_type is not None: - tid_kind = self._make_index(tid_type, tid_type_default, namespace=tid_type_namespace, - reversed=tid_type_reversed, pack=False) + # NOTE: ``tid_type`` defaults to :data:`None`, so ``_make_index`` has no + # enum default to infer the namespace of a :obj:`str` member name from; + # name it here rather than leave the name unresolvable (#2356). + tid_kind = self._make_index( + tid_type, tid_type_default, + namespace=tid_type_namespace if tid_type_namespace is not None else Enum_TaggerID, + reversed=tid_type_reversed, pack=False) elif tid_len == 0: tid_kind = Enum_TaggerID.NULL elif tid_len == 4: @@ -2044,8 +2049,13 @@ def _make_opt_mpl(self, code: 'Enum_Option', opt: 'Optional[Data_MPLOption]' = N seed = opt.seed_id if seed_type is not None: - kind = self._make_index(seed_type, seed_type_default, namespace=seed_type_namespace, - reversed=seed_type_reversed, pack=False) + # NOTE: ``seed_type`` defaults to :data:`None`, so ``_make_index`` has no + # enum default to infer the namespace of a :obj:`str` member name from; + # name it here rather than leave the name unresolvable (#2356). + kind = self._make_index( + seed_type, seed_type_default, + namespace=seed_type_namespace if seed_type_namespace is not None else Enum_SeedID, + reversed=seed_type_reversed, pack=False) elif seed is None or isinstance(seed, ipaddress.IPv6Address): kind = Enum_SeedID.IPV6_SOURCE_ADDRESS else: diff --git a/tests/protocols/test_make_index_str_unit.py b/tests/protocols/test_make_index_str_unit.py index b0e1c4c72f..5b21b48063 100644 --- a/tests/protocols/test_make_index_str_unit.py +++ b/tests/protocols/test_make_index_str_unit.py @@ -9,6 +9,12 @@ enumeration class of the argument's default value, and builds the same bytes as the member itself and as its integer value. +GitHub issue #2356 is the other half: an argument carrying an optional field +defaults to :data:`None`, which is no enumeration member, so there is nothing +for that inference to read. Such a maker names its enumeration at the +:meth:`~pcapkit.protocols.protocol.ProtocolBase._make_index` call instead, and +:data:`NONE_DEFAULT_CASES` carries one case per argument that does. + Every case builds its own packet in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`, so they belong to the live @@ -38,6 +44,27 @@ def _tcp_alternate_checksum(**kwargs: 'Any') -> 'Any': ) +def _ipv6_option(protocol: 'str', option: 'str') -> 'Any': + """A builder putting one option into an IPv6 extension header. + + Args: + protocol: import path of the extension header class + option: :class:`~pcapkit.const.ipv6.option.Option` member name + + Returns: + A callable taking the option's own keywords and returning the built + header, so that an option argument is reached the way ``make()`` + reaches it. + + """ + def build(**kwargs: 'Any') -> 'Any': + code = getattr(_attr('pcapkit.const.ipv6.option.Option'), option) + transtype = _attr('pcapkit.const.reg.transtype.TransType') + return _attr(protocol)(options=[(code, kwargs)], + next=transtype.IPv6_NoNxt, payload=b'') + return build + + #: One ``make()`` per protocol family: #: (protocol class or builder, argument, enum class, non-default member). CASES = { @@ -56,6 +83,27 @@ def _tcp_alternate_checksum(**kwargs: 'Any') -> 'Any': } +#: One ``make()`` per argument whose own default is :data:`None`, so that no enum +#: default is there for the namespace to be inferred from (#2356): (builder, +#: argument, enum class, member, the other keywords the option needs). Each +#: member is one the maker would not have arrived at on its own, so a name that +#: went unresolved cannot pass as a name that resolved. +NONE_DEFAULT_CASES = { + 'internet/hopopt MPL': (_ipv6_option('pcapkit.protocols.internet.hopopt.HOPOPT', 'MPL_Option'), + 'seed_type', 'pcapkit.const.ipv6.seed_id.SeedID', + 'SEEDID_64_BIT_UNSIGNED_INTEGER', {'seed': 5, 'seq': 7}), + 'internet/ipv6_opts MPL': (_ipv6_option('pcapkit.protocols.internet.ipv6_opts.IPv6_Opts', 'MPL_Option'), + 'seed_type', 'pcapkit.const.ipv6.seed_id.SeedID', + 'SEEDID_64_BIT_UNSIGNED_INTEGER', {'seed': 5, 'seq': 7}), + 'internet/hopopt SMF_DPD': (_ipv6_option('pcapkit.protocols.internet.hopopt.HOPOPT', 'SMF_DPD'), + 'tid_type', 'pcapkit.const.ipv6.tagger_id.TaggerID', + 'DEFAULT', {'tid': b'\x0a\x0b\x0c\x0d', 'id': b'\xaa\xbb'}), + 'internet/ipv6_opts SMF_DPD': (_ipv6_option('pcapkit.protocols.internet.ipv6_opts.IPv6_Opts', 'SMF_DPD'), + 'tid_type', 'pcapkit.const.ipv6.tagger_id.TaggerID', + 'DEFAULT', {'tid': b'\x0a\x0b\x0c\x0d', 'id': b'\xaa\xbb'}), +} + + class TestMakeIndexStr(unittest.TestCase): """Pin member-name resolution in ``_make_index`` across families.""" @@ -74,12 +122,54 @@ def test_member_name_matches_member_and_int(self) -> None: # the member is not the default, so the name was not ignored self.assertNotEqual(build().data, expected) + def test_member_name_resolves_where_the_argument_defaults_to_none(self) -> None: + """An argument whose own default is :data:`None` resolves a name too. See #2356. + + The namespace is inferred from the enumeration class of the argument's + default value, which an argument defaulting to :data:`None` -- because + the field it carries is optional -- does not have. Such a maker names + its enumeration at the call instead, so that a member name resolves + without the caller also passing a ``*_namespace``. + """ + for case, (build, arg, enum_path, member, extra) in NONE_DEFAULT_CASES.items(): + with self.subTest(case=case): + value = getattr(_attr(enum_path), member) + + expected = build(**extra, **{arg: value}).data + self.assertEqual(build(**extra, **{arg: member}).data, expected) + self.assertEqual(build(**extra, **{arg: int(value)}).data, expected) + # the maker derives the field from its siblings when the argument + # is not given, and derives a different value here + self.assertNotEqual(build(**extra).data, expected) + + def test_http_status_member_name_resolves(self) -> None: + """An HTTP/1.* status code resolves from its member name. See #2356. + + ``status`` defaults to :data:`None`, since a response has a status where + a request has a method and a URI, so its namespace is named at the call. + :meth:`~pcapkit.protocols.protocol.ProtocolBase._make_index` yields the + member's integer, and a start line built from the integer carries no + reason phrase; only the member itself brings one along. + """ + http = _attr('pcapkit.protocols.application.httpv1.HTTP') + status_code = _attr('pcapkit.const.http.status_code.StatusCode') + + self.assertEqual(http(status='CODE_404').data, http(status=404).data) + self.assertEqual(http(status='CODE_404').data, b'HTTP/1.1 404 \r\n\r\n') + self.assertEqual(http(status=status_code.CODE_404).data, b'HTTP/1.1 404 Not Found\r\n\r\n') + def test_unknown_name_still_raises(self) -> None: exc = _attr('pcapkit.utilities.exceptions.ProtocolNotImplemented') ethernet = _attr('pcapkit.protocols.link.ethernet.Ethernet') with self.assertRaises(exc): ethernet(type='No_Such_EtherType') + # a named namespace resolves a member name, not an arbitrary string: an + # HTTP status code member is spelled ``CODE_404``, so ``'OK'`` is none + http = _attr('pcapkit.protocols.application.httpv1.HTTP') + with self.assertRaises(exc): + http(status='OK') + def test_unknown_name_falls_back_to_default(self) -> None: ethernet = _attr('pcapkit.protocols.link.ethernet.Ethernet') proto = ethernet(type='No_Such_EtherType', type_default=0x1234) @@ -138,10 +228,14 @@ def test_without_inferable_enum_still_raises(self) -> None: with self.assertRaises(exc): protocol._make_index('Internet_Protocol_version_6') - # ``status`` defaults to ``None``, so its enum cannot be inferred - http = _attr('pcapkit.protocols.application.httpv1.HTTP') + # an argument defaulting to ``None`` carries no enum to infer from, so a + # maker that names none at the call leaves the name unresolvable + class OptionalArgument(protocol): # type: ignore[misc,valid-type] + def make_optional(self, arg: 'Any' = None, arg_namespace: 'Any' = None) -> 'int': + return self._make_index(arg, namespace=arg_namespace) + with self.assertRaises(exc): - http(status='OK') + OptionalArgument.make_optional(OptionalArgument, arg='Internet_Protocol_version_6') if __name__ == '__main__':