diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index e0b5cbeeb8..dad88d5582 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -687,8 +687,19 @@ class will consider negative value as a placeholder. self.__buffer__[field.name] = b'' elif isinstance(data, bytes): self.__buffer__[field.name] = data - elif isinstance(data, list): - self.__buffer__[field.name] = field.pack(data, packet) + elif isinstance(data, (list, tuple)): + # NOTE: a data model may declare a field ``tuple[...]`` + # rather than ``list[...]`` -- e.g. HIP's ``group_id: + # 'tuple[Group, ...]'`` in pcapkit/protocols/data/internet/ + # hip.py -- and ``_read_*`` then hands one straight back + # here on reconstruction. ``ListField.pack`` only ever + # iterates its argument, so it does not care which of the + # two it gets; rejecting the tuple broke every + # parse-then-reconstruct cycle for such a field. See #476. + # ``list(data)`` is a no-op for an actual list and keeps + # ``ListField.pack``'s own ``Optional[list[_TL]]`` + # signature honest rather than widening it too. + self.__buffer__[field.name] = field.pack(list(data), packet) else: raise ProtocolUnbound(f'unsupported type {type(data)}') continue diff --git a/tests/protocols/schema/test_schema_unit.py b/tests/protocols/schema/test_schema_unit.py index 4cda19844f..c00d12f247 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -99,6 +99,43 @@ def test_schema_pack_unpack_and_mapping_methods(self) -> None: # above it leaves none of them absent self.assertEqual(list(unpacked), list(FeatureSchema.__fields__)) + def test_list_field_pack_accepts_a_tuple_like_it_accepts_a_list(self) -> None: + """A ``ListField`` value packs identically whether it is a list or a tuple. + + See #476: several data models declare a ``ListField``-backed attribute as + ``tuple[...]`` (HIP's ``group_id``, MH's ``prefixes``/``fid``/``bid``, and + others), and ``_read_*`` hands one straight back to ``_make_*`` on a + parse-then-reconstruct cycle. Before the fix, :meth:`Schema.pack + ` accepted a :obj:`list` but + raised :exc:`ProtocolUnbound` on the tuple -- a case no unit test that + builds the schema directly with a list could ever see. + + """ + NestedSchema, FeatureSchema, _, _, _ = self._make_schema_classes() + from pcapkit.utilities.exceptions import ProtocolUnbound + + as_list = FeatureSchema( + kind=9, maybe=0xAB, peek=0xFE, repeated=[0x10, 0x11], + nested=NestedSchema(marker=0x44), payload=b'body', + ) + as_tuple = FeatureSchema( + kind=9, maybe=0xAB, peek=0xFE, repeated=(0x10, 0x11), + nested=NestedSchema(marker=0x44), payload=b'body', + ) + self.assertEqual(bytes(as_tuple), bytes(as_list)) + self.assertEqual(bytes(as_tuple), b'\x09\xab\x10\x11\x44\x00\x00body') + + # The branch still rejects what it always rejected: a tuple is accepted + # because it is a sequence ``ListField.pack`` can iterate, not because + # the check grew permissive. A :obj:`str` is also a sequence but is not + # what any data model here declares, so it stays out, same as + # ``object()`` did before this fix. + with self.assertRaises(ProtocolUnbound): + bytes(FeatureSchema( + kind=1, repeated='xy', # type: ignore[arg-type] + nested=NestedSchema(marker=0x33), payload=b'', + )) + def test_schema_update_unknown_fields_and_builtin_field_mapping(self) -> None: _, _, _, _, BuiltinNameSchema = self._make_schema_classes() diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index ab8b7977b1..e3a709a0c7 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -10,8 +10,10 @@ That third step is what this module exists for. A construct-then-parse test passes for a ``_make_*`` that takes only keyword arguments and cannot consume the data model its own ``_read_*`` produced -- which is a defect that ships, and -one this suite had no way to see. Sixteen HIP parameters are in exactly that -state today. +one this suite had no way to see. Sixteen HIP parameters used to be in exactly +that state, until :class:`~pcapkit.protocols.schema.schema.Schema`'s +``ListField`` pack branch was widened to accept the ``tuple`` their own data +models declare, not just ``list``. See #476. The case list and the cycle both live in :file:`examples/generators/options.py`, next to the generator that turns the @@ -321,38 +323,6 @@ class Gap(NamedTuple): 'pcapkit/protocols/internet/hip.py:822 -- Parameter.registry[128] is ' 'UnassignedParameter, because R1CounterParameter declares code=129 only'), - # Sixteen parameters whose ``_read_param_*`` stores a list-valued field as a - # tuple, which ``_make_param_*`` passes straight back to a ``ListField`` - # that accepts only a list. This is the class of defect the reconstruct step - # exists to find: each one constructs and parses perfectly. - # - # TRANSPORT_FORMAT_LIST briefly left this group during #466's review: an - # early revision reused NAT_TRAVERSAL_MODE/ESP_TRANSFORM/HIP_TRANSPORT_MODE's - # shared ``two_octet_prefix_list_len`` guard for this parameter's ``formats`` - # field too, on the mistaken premise that its ``pkt['len'] - 2`` was - # byte-identical *for the same reason*. It is not: :rfc:`7401` Section - # 5.2.11 defines this parameter's ``Length`` as literally "2x number of TF - # types", with nothing between ``Length`` and the list to subtract -- unlike - # the other three, which each genuinely read a two-octet ``reserved``/ - # ``port`` field first. That reuse turned the parameter's own legitimate - # empty-list encoding (``Length = 0``) into a raise, and separately - # under-read every non-empty list by two octets on parse -- both fixed by - # ``transport_format_list_len`` in pcapkit/protocols/schema/internet/hip.py, - # which sizes the list at ``Length`` exactly. With that corrected, this case - # is back to failing the same way its fifteen siblings do. - **{ - f'hip-parameter/{name}': Gap( - 'RECONSTRUCT', "unsupported type ", - 'pcapkit/protocols/schema/schema.py:624 -- _read_param_* returns a ' - 'tuple where _make_param_* needs a list') - for name in ( - 'ACK', 'DH_GROUP_LIST', 'HIP_CIPHER', 'NAT_TRAVERSAL_MODE', - 'HIT_SUITE_LIST', 'REG_INFO', 'REG_REQUEST', 'REG_RESPONSE', - 'REG_FAILED', 'TRANSPORT_FORMAT_LIST', 'ESP_TRANSFORM', 'ACK_DATA', - 'ROUTE_DST', 'HIP_TRANSPORT_MODE', 'ROUTE_VIA', 'VIA_RVS', - ) - }, - # ``_make_param_encrypted`` passes ``cipher=``, which is not a field of # ``EncryptedParameter`` -- so the cipher id is dropped with an # ``UnknownFieldWarning`` and never reaches the wire. The mismatch itself