diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index 8b5b4dd0c..8f3390fe1 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -850,12 +850,13 @@ def _unregistered_member(namespace: 'Type[StdlibEnum] | Type[AenumEnum]', this way, which is what keeps this safe to return from ``post_process``. A direct call to :meth:`~pcapkit.const.reg.apptype.apptype.AppType.get` for the same port - is a different matter and deliberately so: asking the - registry for a name is an explicit request for a named member, so - it still mints one -- measured on this tree, - ``AppType.get(54321, proto=tcp)`` returns ``PORT_54321_tcp`` and - takes ``TCP.__members__`` from 6147 to 6148, and a second call - with the same port returns that member rather than raising. Two + does not come through here, and it does not mint either: it + builds an unregistered member of its own, through the registry's + ``_unregistered_member`` -- measured on this tree, + ``AppType.get(54321, proto=tcp)`` returns + ````, ``TCP.__members__`` stays at + 6147, and a second call with the same port returns an equal + member rather than the same one. Two unregistered members for the same value also compare equal without being identical, since :class:`~pcapkit.const.reg.apptype.apptype.AppType` and @@ -1087,18 +1088,31 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'AppTy the port itself, when the registry declares none. Notes: - :meth:`~pcapkit.const.reg.apptype.apptype.AppType.get` mints a fresh - member -- via :func:`aenum.extend_enum` -- for any port neither an - existing row nor one of :meth:`_missing_`'s documented IANA spans - accounts for, which in practice means the ephemeral/dynamic range. - Calling it on every parsed port would therefore grow the registry - without bound. This peeks at the registry + This peeks at what :meth:`~pcapkit.const.reg.apptype.apptype.AppType.get` itself would - consult -- its per-port rows via ``__registry__.getlist``, then - its documented spans via ``_missing_`` -- and only calls - :meth:`~pcapkit.const.reg.apptype.apptype.AppType.get` once one of those is - already known to hold, so a genuine miss gets - :meth:`EnumField._unregistered_member` instead of a mint. + consult -- the per-port rows via ``__registry__.getlist``, then the + documented IANA spans via + :meth:`~pcapkit.const.reg.apptype.apptype.AppType._missing_` -- and + only calls ``get`` once one of those is known to hold. It is not a + guard against growth: ``get`` answers a port neither accounts for + with an unregistered member rather than minting one (GitHub issue + :issue:`860`), and the registry does not grow either way + (:issue:`1774`). It exists because that member is not the one a + parse needs. ``get`` builds it through + ``AppType._unregistered_member``, which installs no + ``__reduce_ex__``, so ``pickle.loads(pickle.dumps(...))`` raises + ``ValueError: 'unknown [50000 - tcp]' is not a valid TCP``; and its + ``.name`` is ``'unknown'``, so it dumps as ``TCP::unknown [...]`` + rather than as the ``TCP:: [...]`` of every other + unassigned wire value. A genuine miss therefore builds its member + through :meth:`EnumField._unregistered_member` instead. + + That member carries the registry's own ``__transport__``, as + ``get``'s does, rather than ``self._proto``. The two are the same + member for each of the three schemas, but a composite built by hand + such as ``TransportProtocol.tcp | TransportProtocol.udp`` is a bare + :class:`int` (``3``, which dispatches to SCTP), and reading ``.name`` + off it would raise :exc:`AttributeError` on a miss ``get`` resolves. A port outside this field's own width is rejected *before* any of that, rather than being let through to :meth:`_missing_` and @@ -1149,8 +1163,11 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'AppTy # DictDumper.object_hook renders a member's addon keys straight # out of its ``__dict__`` in insertion order -- so any other # order here would make an unassigned port dump ``port`` before - # ``svc`` while every declared one dumps ``svc`` first. + # ``svc`` while every declared one dumps ``svc`` first. The + # transport is ``owner.__transport__``, as AppType.get uses, + # since ``proto`` may be a bare int with no ``.name`` (#1774). + transport = owner.__transport__ return self._unregistered_member( - owner, f'unknown [{value:d} - {proto.name}]', - svc='unknown', port=value, proto=proto) + owner, f'unknown [{value:d} - {transport.name}]', + svc='unknown', port=value, proto=transport) return self._namespace.get(value, proto=proto) diff --git a/tests/corekit/test_fields_numbers_port_enum_field_peek_1774_unit.py b/tests/corekit/test_fields_numbers_port_enum_field_peek_1774_unit.py new file mode 100644 index 000000000..61459a253 --- /dev/null +++ b/tests/corekit/test_fields_numbers_port_enum_field_peek_1774_unit.py @@ -0,0 +1,167 @@ +# -*- coding: utf-8 -*- +"""Why :meth:`~pcapkit.corekit.fields.numbers.PortEnumField.post_process` still peeks. + +GitHub issue #1774. The field looks a port up in the registry rows and in +``AppType._missing_``'s spans before calling ``AppType.get``, so that a genuine +miss builds its member through ``EnumField._unregistered_member``. That peek was +written to stop ``get`` minting a member per ephemeral port. Since #860 ``get`` +no longer mints, so the growth it guarded against is gone; but the member ``get`` +builds on a miss is still not the one a parse needs. These tests pin both +halves, so that the peek's documented reason fails here the day it stops being +true, and pin that a composite ``proto`` resolves a miss as ``get`` does. + +""" +import pickle +import unittest +from typing import TYPE_CHECKING + +from tests._support import reimport_once_per_class + +if TYPE_CHECKING: + from typing import Any + +#: An unassigned port on all four transports, in none of ``_missing_``'s spans. +PORT = 50000 + + +class PortEnumFieldPeekTests(unittest.TestCase): + """The peek's reason, over every transport registry.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def _protos(self) -> 'tuple[Any, ...]': + from pcapkit.const.reg.apptype import TransportProtocol + + return (TransportProtocol.tcp, TransportProtocol.udp, + TransportProtocol.sctp, TransportProtocol.dccp) + + def _field(self, proto: 'Any') -> 'Any': + from pcapkit.const.reg.apptype import AppType + from pcapkit.corekit.fields.numbers import PortEnumField + + return PortEnumField(length=2, namespace=AppType, proto=proto) + + @staticmethod + def _tables(owner: 'Any') -> 'tuple[Any, ...]': + by_value = dict(owner._value2member_map_) # pylint: disable=protected-access + rows = [(port, list(owner.__registry__.getlist(port))) for port in owner.__registry__] + return (dict(owner.__members__), by_value, rows) + + def test_neither_the_field_nor_get_grows_the_registry(self) -> None: + """The peek's original reason is gone: ``get`` itself no longer mints.""" + from pcapkit.const.reg.apptype import AppType + + for proto in self._protos(): + with self.subTest(proto=proto.name): + owner = AppType.__registries__[proto] + before = self._tables(owner) + + via_get = AppType.get(PORT, proto=proto) + via_field = self._field(proto).unpack(PORT.to_bytes(2, 'big'), {}) + + self.assertEqual(self._tables(owner), before) + for member in (via_get, via_field): + self.assertIsInstance(member, owner) + self.assertEqual((member.svc, member.port, member.proto), + ('unknown', PORT, proto)) + + def test_a_miss_through_the_field_pickles_and_renders_as_unassigned(self) -> None: + """What the peek buys: the member a parse returns survives pickling and + dumps as ````, like every other unassigned wire value.""" + from pcapkit.const.reg.apptype import AppType + from pcapkit.dumpkit.common import render_enum + + for proto in self._protos(): + owner = AppType.__registries__[proto] + member = self._field(proto).unpack(PORT.to_bytes(2, 'big'), {}) + with self.subTest(proto=proto.name): + self.assertEqual(member.name, '') + expected = f'{owner.__name__}:: [unknown [{PORT} - {proto.name}]]' + self.assertEqual(render_enum(member), expected) + for protocol in range(pickle.HIGHEST_PROTOCOL + 1): + with self.subTest(proto=proto.name, protocol=protocol): + restored = pickle.loads(pickle.dumps(member, protocol)) + self.assertIsInstance(restored, owner) + self.assertEqual((restored.svc, restored.port, restored.proto), + ('unknown', PORT, proto)) + + def test_gets_own_member_for_a_miss_is_not_that_member(self) -> None: + """Why the field cannot just call ``get``: its miss member has no reducer + and another name. If this starts failing, ``get``'s member has become + what a parse needs, and the peek can go -- see #1774.""" + from pcapkit.const.reg.apptype import AppType + + for proto in self._protos(): + with self.subTest(proto=proto.name): + member = AppType.get(PORT, proto=proto) + + self.assertEqual(member.name, 'unknown') + with self.assertRaises(ValueError): + pickle.loads(pickle.dumps(member)) + + def test_a_composite_proto_resolves_a_miss_as_get_does(self) -> None: + """``tcp | udp`` is a bare :class:`int` (``3``, which is SCTP's value). + + ``get`` dispatches it to SCTP for a hit and a miss alike. The field's + own miss branch used to read ``.name`` off it, raising + :exc:`AttributeError` for a port ``get`` resolves; it now takes the + transport from the registry, as ``get`` does. + + """ + from pcapkit.const.reg.apptype import AppType, TransportProtocol + + for label, proto in (('tcp|udp', TransportProtocol.tcp | TransportProtocol.udp), + ('int 1', 1)): + with self.subTest(proto=label): + self.assertIsInstance(proto, int) + self.assertNotIsInstance(proto, TransportProtocol) + field = self._field(proto) + expected = AppType.get(PORT, proto=proto) + owner = type(expected) + before = self._tables(owner) + + miss = field.unpack(PORT.to_bytes(2, 'big'), {}) + hit = field.unpack(b'\x00\x50', {}) # 80 + + self.assertIsInstance(miss, owner) + self.assertEqual((miss.svc, miss.port, miss.proto), + (expected.svc, expected.port, expected.proto)) + self.assertEqual(miss.name, '') + self.assertIs(hit, AppType.get(80, proto=proto)) + self.assertEqual(self._tables(owner), before) + + def test_a_str_proto_resolves_a_miss_as_get_does(self) -> None: + """``'tcp'`` names its registry, as ``get`` lets it, hit and miss alike.""" + from pcapkit.const.reg.apptype import AppType, TransportProtocol + + tcp = AppType.__registries__[TransportProtocol.tcp] + field = self._field('tcp') + before = self._tables(tcp) + + miss = field.unpack(PORT.to_bytes(2, 'big'), {}) + hit = field.unpack(b'\x00\x50', {}) # 80 + + self.assertIsInstance(miss, tcp) + self.assertEqual((miss.svc, miss.port, miss.proto), + ('unknown', PORT, TransportProtocol.tcp)) + self.assertEqual(miss.name, '') + self.assertIs(hit, AppType.get(80, proto='tcp')) + self.assertEqual(self._tables(tcp), before) + + def test_a_composite_naming_no_registry_is_refused_as_get_refuses_it(self) -> None: + """``tcp | dccp`` is ``5``, which names no registry, hit or miss.""" + from pcapkit.const.reg.apptype import AppType, TransportProtocol + + proto = TransportProtocol.tcp | TransportProtocol.dccp + field = self._field(proto) + for port in (80, PORT): + with self.subTest(port=port): + with self.assertRaises(ValueError): + AppType.get(port, proto=proto) + with self.assertRaises(ValueError): + field.unpack(port.to_bytes(2, 'big'), {}) + + +if __name__ == '__main__': + unittest.main()