From 5c03bf41381a55f402f44090a172b73f2fa5d714 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 27 Oct 2026 11:57:02 -0400 Subject: [PATCH] refactor(foundation): declare each registry's code kind as a class attribute, not a module table (#2343) Problem: _DESTINATION_KINDS mapped a protocol class to the Enum registry _check_code validates its code against, found by walking the destination's MRO for whichever ancestor owns __proto__ and looking that owner up in the table. The pairing lived apart from the __proto__ it describes, and a new destination needed both a new __proto__ and a new row in a module dict elsewhere. Fix: each of the seven destination classes (Link, Internet, Frame, PCAPNG, TCP, UDP, SCTP) now declares its own __proto_key__ class attribute, alongside __proto__, pairing its Enum registry with whether a bare int is also valid. register_protocol_code reads it straight off the owner the MRO-walk already finds (via vars(), not getattr, so a class keeping a registry of its own -- Loopback, IPX -- does not inherit its parent's pairing). _check_code also takes the destination class explicitly now, so its error names which registry refused, not just which enum: "TCP: code must be ... not True" rather than "code must be ... not True". Every other call site of _check_code (register_ethertype, register_transtype, every option/parameter registrar) passes its own target the same way. Two call sites write into two registries keyed alike and pass one of the two as target, rather than checking twice for no behavioural difference: register_linktype (Frame and PCAPNG) names Frame; register_apptype (TCP and/or UDP, resolved after the check) names TCP. Test: test_a_registry_of_its_own_does_not_inherit_its_parent_pairing pins the Loopback/IPX case; test_a_rejection_names_the_destination_registry and test_every_rejection_names_the_destination_registry (24 subtests, one per call site) pin the new error prefix; test_every_destination_kind_row_is_enforced now walks every ProtocolBase subclass declaring __proto_key__ rather than a fixed dict's keys, so a new undeclared pairing is caught too. --- .../pcapkit/protocols/internet/internet.rst | 2 + docs/source/pcapkit/protocols/link/link.rst | 2 + docs/source/pcapkit/protocols/misc/pcap.rst | 2 + docs/source/pcapkit/protocols/misc/pcapng.rst | 2 + .../pcapkit/protocols/transport/sctp.rst | 2 + .../pcapkit/protocols/transport/tcp.rst | 2 + .../pcapkit/protocols/transport/udp.rst | 2 + pcapkit/foundation/registry/protocols.py | 115 ++++++++++-------- pcapkit/protocols/internet/internet.py | 15 ++- pcapkit/protocols/link/link.py | 16 ++- pcapkit/protocols/misc/pcap/frame.py | 14 ++- pcapkit/protocols/misc/pcapng.py | 16 ++- pcapkit/protocols/transport/sctp.py | 13 +- pcapkit/protocols/transport/tcp.py | 16 ++- pcapkit/protocols/transport/udp.py | 16 ++- .../test_registrar_code_check_unit.py | 42 ++++++- .../test_protocol_code_registration_unit.py | 112 ++++++++++++++--- 17 files changed, 307 insertions(+), 82 deletions(-) diff --git a/docs/source/pcapkit/protocols/internet/internet.rst b/docs/source/pcapkit/protocols/internet/internet.rst index c49cbf69d6..4861661ff5 100644 --- a/docs/source/pcapkit/protocols/internet/internet.rst +++ b/docs/source/pcapkit/protocols/internet/internet.rst @@ -28,6 +28,8 @@ which is a base class for internet layer protocols, e.g. :class:`~pcapkit.protoc :no-value: .. autoattribute:: __proto__ :no-value: + .. autoattribute:: __proto_key__ + :no-value: .. autoattribute:: _exthdr_depth .. autodata:: pcapkit.protocols.internet.internet._exthdr_rebuilt diff --git a/docs/source/pcapkit/protocols/link/link.rst b/docs/source/pcapkit/protocols/link/link.rst index 879eed0b4e..203f94efdd 100644 --- a/docs/source/pcapkit/protocols/link/link.rst +++ b/docs/source/pcapkit/protocols/link/link.rst @@ -22,3 +22,5 @@ and :class:`~pcapkit.protocols.link.vlan.VLAN`. .. autoattribute:: __layer__ .. autoattribute:: __proto__ :no-value: + .. autoattribute:: __proto_key__ + :no-value: diff --git a/docs/source/pcapkit/protocols/misc/pcap.rst b/docs/source/pcapkit/protocols/misc/pcap.rst index 67e3f32d57..57041c2f99 100644 --- a/docs/source/pcapkit/protocols/misc/pcap.rst +++ b/docs/source/pcapkit/protocols/misc/pcap.rst @@ -143,6 +143,8 @@ whose structure is described as below: .. autoattribute:: __proto__ :no-value: + .. autoattribute:: __proto_key__ + :no-value: .. automethod:: __post_init__ .. automethod:: __index__ diff --git a/docs/source/pcapkit/protocols/misc/pcapng.rst b/docs/source/pcapkit/protocols/misc/pcapng.rst index cd0bac3eea..9d33b67751 100644 --- a/docs/source/pcapkit/protocols/misc/pcapng.rst +++ b/docs/source/pcapkit/protocols/misc/pcapng.rst @@ -194,6 +194,8 @@ which implements an extractor for PCAP-NG file format [*]_, and the .. autoattribute:: __proto__ :no-value: + .. autoattribute:: __proto_key__ + :no-value: .. autoattribute:: __block__ :no-value: .. autoattribute:: __option__ diff --git a/docs/source/pcapkit/protocols/transport/sctp.rst b/docs/source/pcapkit/protocols/transport/sctp.rst index a394c1611b..4bce600335 100644 --- a/docs/source/pcapkit/protocols/transport/sctp.rst +++ b/docs/source/pcapkit/protocols/transport/sctp.rst @@ -139,6 +139,8 @@ Octets Bits Name Description .. autoattribute:: __proto__ :no-value: + .. autoattribute:: __proto_key__ + :no-value: .. autoattribute:: __chunk__ :no-value: .. autoattribute:: __parameter__ diff --git a/docs/source/pcapkit/protocols/transport/tcp.rst b/docs/source/pcapkit/protocols/transport/tcp.rst index 50aab04830..08694b8c2b 100644 --- a/docs/source/pcapkit/protocols/transport/tcp.rst +++ b/docs/source/pcapkit/protocols/transport/tcp.rst @@ -131,6 +131,8 @@ Octets Bits Name Description .. autoattribute:: __proto__ :no-value: + .. autoattribute:: __proto_key__ + :no-value: .. autoattribute:: __option__ :no-value: .. autoattribute:: __mp_option__ diff --git a/docs/source/pcapkit/protocols/transport/udp.rst b/docs/source/pcapkit/protocols/transport/udp.rst index 17986ccf63..2f7777ff8f 100644 --- a/docs/source/pcapkit/protocols/transport/udp.rst +++ b/docs/source/pcapkit/protocols/transport/udp.rst @@ -33,6 +33,8 @@ Octets Bits Name Description .. autoattribute:: __proto__ :no-value: + .. autoattribute:: __proto_key__ + :no-value: .. automethod:: __index__ diff --git a/pcapkit/foundation/registry/protocols.py b/pcapkit/foundation/registry/protocols.py index fed3cc25ab..a910e76193 100644 --- a/pcapkit/foundation/registry/protocols.py +++ b/pcapkit/foundation/registry/protocols.py @@ -423,25 +423,6 @@ def _register_name(protocol: 'Type[ProtocolBase]', *targets: 'tuple[type, Any]') Enum_LinkType: (Frame, PCAPNG), } -#: Key type of each ``__proto__`` dispatch registry a ``code=`` target can name. -#: -#: Maps the class owning the registry to the enum its typed registrar checks -#: ``code`` against, and whether a bare :class:`int` is a valid key there too -- -#: the same :func:`_check_code` arguments that registrar passes. A destination is -#: looked up by the class it inherits ``__proto__`` from, so a subclass sharing -#: :class:`Link`'s registry is checked as :class:`Link` is, while one keeping a -#: registry of its own, such as :class:`~pcapkit.protocols.link.loopback.Loopback`, -#: names no entry here and is not checked (:issue:`2268`). -_DESTINATION_KINDS: 'dict[Type[ProtocolBase], tuple[Type[EnumRegistry], bool]]' = { - Link: (Enum_EtherType, False), # type: ignore[type-abstract] - Internet: (Enum_TransType, False), # type: ignore[type-abstract] - Frame: (Enum_LinkType, False), - PCAPNG: (Enum_LinkType, False), - TCP: (Enum_AppType, True), - UDP: (Enum_AppType, True), - SCTP: (Enum_PayloadProtocolIdentifier, True), -} - def _iter_code_targets(code: 'Any') -> 'Iterator[tuple[Type[ProtocolBase], Any]]': """Flatten a ``code=`` argument into ``(destination, key)`` pairs. @@ -549,18 +530,34 @@ def register_protocol_code(protocol: 'Type[ProtocolBase]', code: 'Any') -> 'None an enum member's type names no known destination registry -- inference refuses rather than guesses; or if a key is not of the type its destination's registry is keyed by, as its typed registrar - would reject it, e.g. ``{Frame: TransType.ICMP}``. Every target is - checked before any is registered, so a rejected ``code`` leaves - every registry as it was (:issue:`2268`). + would reject it, e.g. ``{Frame: TransType.ICMP}``. What a registry is + keyed by is read from the + :attr:`__proto_key__ ` + declared by the class owning it, so a destination sharing another's + registry is checked as its owner is, and one whose owner declares no + pairing is not checked. Every target is checked before any is + registered, so a rejected ``code`` leaves every registry as it was + (:issue:`2268`). """ targets = list(_iter_code_targets(code)) for destination, key in targets: owner = next((klass for klass in getattr(destination, '__mro__', ()) if '__proto__' in vars(klass)), None) - if owner in _DESTINATION_KINDS: - kind, number = _DESTINATION_KINDS[owner] - _check_code(key, kind, number=number) + if owner is None: + continue + + # NOTE: the pairing is read out of the owner's own ``vars()`` rather than + # with ``getattr``, which walks the MRO and would hand a class keeping a + # registry of its own its parent's pairing -- + # :attr:`Loopback.__proto__ ` + # is keyed by address family rather than by EtherType, so inheriting + # :class:`Link`'s would reject every key it does accept. A class is + # checked against the registry it owns, or not at all. + pairing = vars(owner).get('__proto_key__') + if pairing is not None: + kind, number = pairing + _check_code(key, kind, owner, number=number) for destination, key in targets: destination.register(key, protocol) @@ -568,7 +565,8 @@ def register_protocol_code(protocol: 'Type[ProtocolBase]', code: 'Any') -> 'None destination.__name__, key) -def _check_code(code: 'Any', kind: 'type', *, number: 'bool' = False) -> 'None': +def _check_code(code: 'Any', kind: 'type', target: 'Type[ProtocolBase]', *, + number: 'bool' = False) -> 'None': """Reject a registration key that is not a member of the registrar's enum. Each registrar typed on one enum calls this before touching any registry, @@ -582,6 +580,10 @@ def _check_code(code: 'Any', kind: 'type', *, number: 'bool' = False) -> 'None': Args: code: Registration key handed to the registrar. kind: Enum the registrar is typed on. + target: Protocol class whose registry ``code`` is destined for, named in + the error message so a rejection says *whose* registry rejected the + key rather than only what it wanted -- the registrars share one + message, and several of them are typed on the same enum. number: Whether a bare :class:`int` is a valid key too, as for the registrars keyed on a port number or a payload protocol identifier. @@ -600,8 +602,9 @@ def _check_code(code: 'Any', kind: 'type', *, number: 'bool' = False) -> 'None': return expected = f'an int or a member of {kind.__name__}' if number else f'a member of {kind.__name__}' if member: - raise RegistryError(f'code must be {expected}, not a member of {type(code).__name__}: {code!r}') - raise RegistryError(f'code must be {expected}, not {code!r}') + raise RegistryError(f'{target.__name__}: code must be {expected}, not a member of ' + f'{type(code).__name__}: {code!r}') + raise RegistryError(f'{target.__name__}: code must be {expected}, not {code!r}') ############################################################################### @@ -646,7 +649,10 @@ def register_linktype(code: 'LinkType', module: 'str | ModuleDescriptor[Protocol subclass. """ - _check_code(code, Enum_LinkType) + # NOTE: this writes into both Frame's and PCAP-NG's registries, which are + # keyed alike, so the key check is the same for either and Frame names it. + # :func:`register_pcap` and :func:`register_pcapng` name one each. + _check_code(code, Enum_LinkType, Frame) if isinstance(module, str): module = cast('ModuleDescriptor[ProtocolBase]', ModuleDescriptor(module, class_)) @@ -691,7 +697,7 @@ def register_pcap(code: 'LinkType', module: 'str | ModuleDescriptor[ProtocolBase subclass. """ - _check_code(code, Enum_LinkType) + _check_code(code, Enum_LinkType, Frame) if isinstance(module, str): module = cast('ModuleDescriptor[ProtocolBase]', ModuleDescriptor(module, class_)) @@ -735,7 +741,7 @@ def register_pcapng(code: 'LinkType', module: 'str | ModuleDescriptor[ProtocolBa subclass. """ - _check_code(code, Enum_LinkType) + _check_code(code, Enum_LinkType, PCAPNG) if isinstance(module, str): module = cast('ModuleDescriptor[ProtocolBase]', ModuleDescriptor(module, class_)) @@ -784,7 +790,7 @@ def register_ethertype(code: 'EtherType', module: 'str | ModuleDescriptor[Protoc subclass. """ - _check_code(code, Enum_EtherType) + _check_code(code, Enum_EtherType, Link) # type: ignore[type-abstract] if isinstance(module, str): module = cast('ModuleDescriptor[ProtocolBase]', ModuleDescriptor(module, class_)) @@ -833,7 +839,7 @@ def register_transtype(code: 'TransType', module: 'str | ModuleDescriptor[Protoc subclass. """ - _check_code(code, Enum_TransType) + _check_code(code, Enum_TransType, Internet) # type: ignore[type-abstract] if isinstance(module, str): module = cast('ModuleDescriptor[ProtocolBase]', ModuleDescriptor(module, class_)) @@ -870,7 +876,7 @@ def register_ipv4_option(code: 'IPv4_OptionNumber', meth: 'str | tuple[IPv4_Opti is a name with no matching ``_read_opt_*`` method. """ - _check_code(code, IPv4_OptionNumber) + _check_code(code, IPv4_OptionNumber, IPv4) if isinstance(meth, str) and not hasattr(IPv4, f'_read_opt_{meth}'): raise RegistryError('method must be a valid IPv4 option parser function') @@ -904,7 +910,7 @@ def register_hip_parameter(code: 'HIP_Parameter', meth: 'str | tuple[HIP_Paramet is a name with no matching ``_read_param_*`` method. """ - _check_code(code, HIP_Parameter) + _check_code(code, HIP_Parameter, HIP) if isinstance(meth, str) and not hasattr(HIP, f'_read_param_{meth}'): raise RegistryError('method must be a valid HIP parameter parser function') @@ -938,7 +944,7 @@ def register_hopopt_option(code: 'IPv6_Option', meth: 'str | tuple[HOPOPT_Option is a name with no matching ``_read_opt_*`` method. """ - _check_code(code, IPv6_Option) + _check_code(code, IPv6_Option, HOPOPT) if isinstance(meth, str) and not hasattr(HOPOPT, f'_read_opt_{meth}'): raise RegistryError('method must be a valid HOPOPT option parser function') @@ -972,7 +978,7 @@ def register_ipv6_opts_option(code: 'IPv6_Option', meth: 'str | tuple[IPv6_Opts_ is a name with no matching ``_read_opt_*`` method. """ - _check_code(code, IPv6_Option) + _check_code(code, IPv6_Option, IPv6_Opts) if isinstance(meth, str) and not hasattr(IPv6_Opts, f'_read_opt_{meth}'): raise RegistryError('method must be a valid IPv6-Opts option parser function') @@ -1006,7 +1012,7 @@ def register_ipv6_route_routing(code: 'IPv6_Routing', meth: 'str | tuple[IPv6_Ro is a name with no matching ``_read_data_type_*`` method. """ - _check_code(code, IPv6_Routing) + _check_code(code, IPv6_Routing, IPv6_Route) if isinstance(meth, str) and not hasattr(IPv6_Route, f'_read_data_type_{meth}'): raise RegistryError('method must be a valid IPv6-Route routing data parser function') @@ -1040,7 +1046,7 @@ def register_mh_message(code: 'MH_Packet', meth: 'str | tuple[MH_PacketParser, M is a name with no matching ``_read_msg_*`` method. """ - _check_code(code, MH_Packet) + _check_code(code, MH_Packet, MH) if isinstance(meth, str) and not hasattr(MH, f'_read_msg_{meth}'): raise RegistryError('method must be a valid MH message type parser function') @@ -1074,7 +1080,7 @@ def register_mh_option(code: 'MH_Option', meth: 'str | tuple[MH_OptionParser, MH is a name with no matching ``_read_opt_*`` method. """ - _check_code(code, MH_Option) + _check_code(code, MH_Option, MH) if isinstance(meth, str) and not hasattr(MH, f'_read_opt_{meth}'): raise RegistryError('method must be a valid MH option parser function') @@ -1113,7 +1119,7 @@ def register_mh_extension(code: 'MH_CGAExtension', meth: 'str | tuple[MH_Extensi that take one (:issue:`516`). """ - _check_code(code, MH_CGAExtension) + _check_code(code, MH_CGAExtension, MH) if isinstance(meth, str) and not hasattr(MH, f'_read_ext_{meth}'): raise RegistryError('method must be a valid MH CGA extension function') @@ -1227,7 +1233,12 @@ def register_apptype(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor protocol per argument, with no alias (:issue:`806`). """ - _check_code(code, Enum_AppType, number=True) + # NOTE: which of TCP's and UDP's registries this writes into depends on + # ``*transport``, which is resolved below -- after this check, so a bad + # ``code`` is still reported before a bad transport protocol. The two are + # keyed alike, so the check itself is the same for either and TCP names it. + # :func:`register_tcp` and :func:`register_udp` name one each. + _check_code(code, Enum_AppType, TCP, number=True) # NOTE: ``class_`` sits at the sibling position -- positional, ahead of # ``*transport`` -- per maintainer ruling, so a third positional that is @@ -1356,7 +1367,7 @@ def register_tcp(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor[Pro member of another enum. """ - _check_code(code, Enum_AppType, number=True) + _check_code(code, Enum_AppType, TCP, number=True) if isinstance(code, Enum_AppType): code = code.port if isinstance(module, str): @@ -1395,7 +1406,7 @@ def register_tcp_option(code: 'TCP_Option', meth: 'str | tuple[TCP_OptionParser, is a name with no matching ``_read_mode_*`` method. """ - _check_code(code, TCP_Option) + _check_code(code, TCP_Option, TCP) if isinstance(meth, str) and not hasattr(TCP, f'_read_mode_{meth}'): raise RegistryError('method must be a TCP option parser function') @@ -1429,7 +1440,7 @@ def register_tcp_mp_option(code: 'TCP_MPTCPOption', meth: 'str | tuple[TCP_MPOpt is a name with no matching ``_read_mptcp_*`` method. """ - _check_code(code, TCP_MPTCPOption) + _check_code(code, TCP_MPTCPOption, TCP) if isinstance(meth, str) and not hasattr(TCP, f'_read_mptcp_{meth}'): raise RegistryError('method must be a MPTCP option parser function') @@ -1470,7 +1481,7 @@ def register_udp(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor[Pro member of another enum. """ - _check_code(code, Enum_AppType, number=True) + _check_code(code, Enum_AppType, UDP, number=True) if isinstance(code, Enum_AppType): code = code.port if isinstance(module, str): @@ -1524,7 +1535,7 @@ def register_sctp(code: 'int | SCTP_PayloadProtocolIdentifier', module: 'str | M :class:`~pcapkit.const.reg.apptype.sctp.SCTP` port enum. """ - _check_code(code, Enum_PayloadProtocolIdentifier, number=True) + _check_code(code, Enum_PayloadProtocolIdentifier, SCTP, number=True) if isinstance(module, str): module = cast('ModuleDescriptor[ProtocolBase]', ModuleDescriptor(module, class_)) @@ -1566,7 +1577,7 @@ def register_http_frame(code: 'HTTP_Frame', meth: 'str | tuple[HTTP_FrameParser, is a name with no matching ``_read_http_*`` method. """ - _check_code(code, HTTP_Frame) + _check_code(code, HTTP_Frame, HTTPv2) if isinstance(meth, str) and not hasattr(HTTPv2, f'_read_http_{meth}'): raise RegistryError('method must be a frame parser function') @@ -1605,7 +1616,7 @@ def register_pcapng_block(code: 'PCAPNG_BlockType', meth: 'str | tuple[PCAPNG_Bl is a name with no matching ``_read_block_*`` method. """ - _check_code(code, PCAPNG_BlockType) + _check_code(code, PCAPNG_BlockType, PCAPNG) if isinstance(meth, str) and not hasattr(PCAPNG, f'_read_block_{meth}'): raise RegistryError('method must be a block parser function') @@ -1639,7 +1650,7 @@ def register_pcapng_option(code: 'PCAPNG_OptionType', meth: 'str | tuple[PCAPNG_ is a name with no matching ``_read_option_*`` method. """ - _check_code(code, PCAPNG_OptionType) + _check_code(code, PCAPNG_OptionType, PCAPNG) if isinstance(meth, str) and not hasattr(PCAPNG, f'_read_option_{meth}'): raise RegistryError('method must be a option parser function') @@ -1673,7 +1684,7 @@ def register_pcapng_record(code: 'PCAPNG_RecordType', meth: 'str | tuple[PCAPNG_ is a name with no matching ``_read_record_*`` method. """ - _check_code(code, PCAPNG_RecordType) + _check_code(code, PCAPNG_RecordType, PCAPNG) if isinstance(meth, str) and not hasattr(PCAPNG, f'_read_record_{meth}'): raise RegistryError('method must be a name resolution record parser function') @@ -1707,7 +1718,7 @@ def register_pcapng_secrets(code: 'PCAPNG_SecretsType', meth: 'str | tuple[PCAPN is a name with no matching ``_read_secrets_*`` method. """ - _check_code(code, PCAPNG_SecretsType) + _check_code(code, PCAPNG_SecretsType, PCAPNG) if isinstance(meth, str) and not hasattr(PCAPNG, f'_read_secrets_{meth}'): raise RegistryError('method must be a decryption secrets parser function') diff --git a/pcapkit/protocols/internet/internet.py b/pcapkit/protocols/internet/internet.py index 2498f0f979..a64b5c769e 100644 --- a/pcapkit/protocols/internet/internet.py +++ b/pcapkit/protocols/internet/internet.py @@ -28,10 +28,11 @@ from pcapkit.utilities.warnings import ProtocolWarning, RegistryWarning, warn if TYPE_CHECKING: - from typing import IO, Any, Optional, Type + from typing import IO, Any, ClassVar, Optional, Type from typing_extensions import Literal, Self + from pcapkit.corekit.enum import EnumRegistry from pcapkit.protocols.data.data import Data __all__ = ['Internet'] @@ -138,6 +139,18 @@ class Internet(ProtocolBase[_PT, _ST], Generic[_PT, _ST]): # pylint: disable=ab }, ) + #: What :attr:`__proto__` is keyed by: the enum a key must be a member of, + #: and whether a bare :class:`int` is a valid key there too. Declared by the + #: class *owning* the registry, and read from it by + #: :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` so a + #: ``code=`` key is checked exactly as + #: :func:`~pcapkit.foundation.registry.protocols.register_transtype` checks + #: its own -- a subclass sharing this registry is therefore checked as + #: :class:`Internet` is, while :class:`~pcapkit.protocols.internet.ipx.IPX`, + #: which keeps one of its own, declares its own pairing or none at all + #: (:issue:`2268`). + __proto_key__: 'ClassVar[tuple[Type[EnumRegistry], bool]]' = (Enum_TransType, False) + #: int: How many IPv6 extension headers in a row the next layer follows: #: this layer's place in such a chain, or how many headers #: :class:`~pcapkit.protocols.internet.ipv6.IPv6` walked, else ``0``, see diff --git a/pcapkit/protocols/link/link.py b/pcapkit/protocols/link/link.py index ce41a97434..cc4d148afe 100644 --- a/pcapkit/protocols/link/link.py +++ b/pcapkit/protocols/link/link.py @@ -25,10 +25,12 @@ from pcapkit.utilities.warnings import RegistryWarning, warn if TYPE_CHECKING: - from typing import IO, Any, DefaultDict, Optional, Type + from typing import IO, Any, ClassVar, DefaultDict, Optional, Type from typing_extensions import Literal, Self + from pcapkit.corekit.enum import EnumRegistry + __all__ = ['Link'] @@ -99,6 +101,18 @@ class Link(ProtocolBase[_PT, _ST], Generic[_PT, _ST]): # pylint: disable=abstra }, ) # type: DefaultDict[int | Enum_EtherType, ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]] + #: What :attr:`__proto__` is keyed by: the enum a key must be a member of, + #: and whether a bare :class:`int` is a valid key there too. Declared by the + #: class *owning* the registry, and read from it by + #: :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` so a + #: ``code=`` key is checked exactly as + #: :func:`~pcapkit.foundation.registry.protocols.register_ethertype` checks + #: its own -- :class:`~pcapkit.protocols.link.ethernet.Ethernet`, which shares + #: this registry, is therefore checked as :class:`Link` is, while + #: :class:`~pcapkit.protocols.link.loopback.Loopback`, which keeps one of its + #: own, declares its own pairing or none at all (:issue:`2268`). + __proto_key__: 'ClassVar[tuple[Type[EnumRegistry], bool]]' = (Enum_EtherType, False) + ########################################################################## # Properties. ########################################################################## diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index 9bc7dd6c33..11028754f5 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -55,10 +55,11 @@ if TYPE_CHECKING: from datetime import datetime as dt_type from decimal import Decimal - from typing import IO, Any, DefaultDict, Optional, Type + from typing import IO, Any, ClassVar, DefaultDict, Optional, Type from typing_extensions import Literal + from pcapkit.corekit.enum import EnumRegistry from pcapkit.protocols.data.misc.pcap.header import Header as Data_Header from pcapkit.protocols.schema.schema import Schema @@ -115,6 +116,17 @@ class Frame(ProtocolBase[Data_Frame, Schema_Frame], }, ) # type: DefaultDict[Enum_LinkType | int, ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]] + #: What :attr:`__proto__` is keyed by: the enum a key must be a member of, + #: and whether a bare :class:`int` is a valid key there too. Declared by the + #: class *owning* the registry, and read from it by + #: :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` so a + #: ``code=`` key is checked exactly as + #: :func:`~pcapkit.foundation.registry.protocols.register_pcap` checks its own -- + #: :class:`~pcapkit.protocols.misc.pcapng.PCAPNG` keeps a registry of its own, + #: keyed by the same enum, and so declares the same pairing rather than + #: sharing this one (:issue:`2268`). + __proto_key__: 'ClassVar[tuple[Type[EnumRegistry], bool]]' = (Enum_LinkType, False) + ########################################################################## # Properties. ########################################################################## diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index 2abd0b3210..8bfbd05500 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -225,13 +225,14 @@ from decimal import Decimal from enum import IntEnum as StdlibEnum from ipaddress import IPv4Address, IPv4Interface, IPv6Address, IPv6Interface - from typing import (IO, Any, Callable, Counter, DefaultDict, Optional, Tuple, Type, TypeVar, - Union) + from typing import (IO, Any, Callable, ClassVar, Counter, DefaultDict, Optional, Tuple, Type, + TypeVar, Union) from aenum import IntEnum as AenumEnum from mypy_extensions import DefaultArg, KwArg, NamedArg from typing_extensions import Literal + from pcapkit.corekit.enum import EnumRegistry from pcapkit.foundation.engines.pcapng import Context from pcapkit.protocols.schema.misc.pcapng import Option as Schema_Option from pcapkit.protocols.schema.schema import Schema @@ -751,6 +752,17 @@ class PCAPNG(ProtocolBase[Data_PCAPNG, Schema_PCAPNG], }, ) # type: DefaultDict[Enum_LinkType | int, ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]] + #: What :attr:`__proto__` is keyed by: the enum a key must be a member of, + #: and whether a bare :class:`int` is a valid key there too. Declared by the + #: class *owning* the registry, and read from it by + #: :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` so a + #: ``code=`` key is checked exactly as + #: :func:`~pcapkit.foundation.registry.protocols.register_pcapng` checks its own -- + #: :class:`~pcapkit.protocols.misc.pcap.frame.Frame` keeps a registry of its own, + #: keyed by the same enum, and so declares the same pairing rather than + #: sharing this one (:issue:`2268`). + __proto_key__: 'ClassVar[tuple[Type[EnumRegistry], bool]]' = (Enum_LinkType, False) + #: Block type to method mapping. A :obj:`str` value names the #: ``_read_block_${name}`` and ``_make_block_${name}`` methods, which fall #: back to ``_read_block_unknown`` and ``_make_block_unknown`` when not diff --git a/pcapkit/protocols/transport/sctp.py b/pcapkit/protocols/transport/sctp.py index 9a5b66dec1..8a7ee9f53c 100644 --- a/pcapkit/protocols/transport/sctp.py +++ b/pcapkit/protocols/transport/sctp.py @@ -161,12 +161,13 @@ if TYPE_CHECKING: from ipaddress import IPv4Address, IPv6Address - from typing import Any, Callable, DefaultDict, Optional, Sequence, Type, TypeVar + from typing import Any, Callable, ClassVar, DefaultDict, Optional, Sequence, Type, TypeVar from mypy_extensions import DefaultArg, KwArg, NamedArg from typing_extensions import Literal from pcapkit.const.reg.apptype import AppType as Enum_AppType + from pcapkit.corekit.enum import EnumRegistry from pcapkit.protocols.data.data import Data from pcapkit.protocols.data.transport.sctp import Chunk as Data_Chunk from pcapkit.protocols.data.transport.sctp import ErrorCause as Data_ErrorCause @@ -471,6 +472,16 @@ class SCTP(Transport[Data_SCTP, Schema_SCTP], }, ) # type: DefaultDict[int, ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]] + #: What :attr:`__proto__` is keyed by: the enum a key must be a member of, + #: and whether a bare :class:`int` is a valid key there too. Declared by the + #: class *owning* the registry, and read from it by + #: :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` so a + #: ``code=`` key is checked exactly as + #: :func:`~pcapkit.foundation.registry.protocols.register_sctp` checks its own + #: -- a bare :class:`int` is accepted because the registry is keyed by + #: payload protocol identifier, which is a number in its own right (:issue:`2268`). + __proto_key__: 'ClassVar[tuple[Type[EnumRegistry], bool]]' = (Enum_PayloadProtocolIdentifier, True) + #: Chunk type to method mapping, see :meth:`_read_sctp_chunks` and #: :meth:`_make_sctp_chunks`. A :class:`str` value names the methods #: ``_read_chunk_${name}`` and ``_make_chunk_${name}``, falling back to diff --git a/pcapkit/protocols/transport/tcp.py b/pcapkit/protocols/transport/tcp.py index 51bf127972..7c9ade29a9 100644 --- a/pcapkit/protocols/transport/tcp.py +++ b/pcapkit/protocols/transport/tcp.py @@ -43,6 +43,7 @@ import math from typing import TYPE_CHECKING, cast +from pcapkit.const.reg.apptype import AppType as Enum_AppType from pcapkit.const.reg.apptype import TransportProtocol as Enum_TransportProtocol from pcapkit.const.reg.transtype import TransType from pcapkit.const.tcp.checksum import Checksum as Enum_Checksum @@ -141,13 +142,13 @@ from datetime import timedelta from enum import IntEnum as StdlibEnum from ipaddress import IPv4Address, IPv6Address - from typing import Any, Callable, DefaultDict, Optional, Type + from typing import Any, Callable, ClassVar, DefaultDict, Optional, Type from aenum import IntEnum as AenumEnum from mypy_extensions import DefaultArg, KwArg, NamedArg from typing_extensions import Literal - from pcapkit.const.reg.apptype import AppType as Enum_AppType + from pcapkit.corekit.enum import EnumRegistry from pcapkit.protocols.data.transport.tcp import MPTCP as Data_MPTCP from pcapkit.protocols.data.transport.tcp import MPTCPJoin as Data_MPTCPJoin from pcapkit.protocols.data.transport.tcp import Option as Data_Option @@ -369,6 +370,17 @@ class TCP(Transport[Data_TCP, Schema_TCP], }, ) + #: What :attr:`__proto__` is keyed by: the enum a key must be a member of, + #: and whether a bare :class:`int` is a valid key there too. Declared by the + #: class *owning* the registry, and read from it by + #: :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` so a + #: ``code=`` key is checked exactly as + #: :func:`~pcapkit.foundation.registry.protocols.register_tcp` checks its own + #: -- a bare :class:`int` is accepted because the registry is keyed by + #: port number, and an :class:`~pcapkit.const.reg.apptype.apptype.AppType` + #: member carries one (:issue:`2268`). + __proto_key__: 'ClassVar[tuple[Type[EnumRegistry], bool]]' = (Enum_AppType, True) + #: Option code to method mapping, see :meth:`_read_tcp_options` and #: :meth:`_make_tcp_options`. A :class:`str` value names the methods #: ``_read_mode_${name}`` and ``_make_mode_${name}``, falling back to diff --git a/pcapkit/protocols/transport/udp.py b/pcapkit/protocols/transport/udp.py index 6dd61ff53d..224d1c8ecf 100644 --- a/pcapkit/protocols/transport/udp.py +++ b/pcapkit/protocols/transport/udp.py @@ -25,6 +25,7 @@ import collections from typing import TYPE_CHECKING +from pcapkit.const.reg.apptype import AppType as Enum_AppType from pcapkit.const.reg.apptype import TransportProtocol as Enum_TransportProtocol from pcapkit.const.reg.transtype import TransType as Enum_TransType from pcapkit.corekit.module import ModuleDescriptor @@ -33,11 +34,11 @@ from pcapkit.protocols.transport.transport import Transport if TYPE_CHECKING: - from typing import Any, Optional + from typing import Any, ClassVar, Optional, Type from typing_extensions import Literal - from pcapkit.const.reg.apptype import AppType as Enum_AppType + from pcapkit.corekit.enum import EnumRegistry from pcapkit.protocols.protocol import ProtocolBase from pcapkit.protocols.schema.schema import Schema @@ -116,6 +117,17 @@ class UDP(Transport[Data_UDP, Schema_UDP], }, ) + #: What :attr:`__proto__` is keyed by: the enum a key must be a member of, + #: and whether a bare :class:`int` is a valid key there too. Declared by the + #: class *owning* the registry, and read from it by + #: :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` so a + #: ``code=`` key is checked exactly as + #: :func:`~pcapkit.foundation.registry.protocols.register_udp` checks its own + #: -- a bare :class:`int` is accepted because the registry is keyed by + #: port number, and an :class:`~pcapkit.const.reg.apptype.apptype.AppType` + #: member carries one (:issue:`2268`). + __proto_key__: 'ClassVar[tuple[Type[EnumRegistry], bool]]' = (Enum_AppType, True) + ########################################################################## # Properties. ########################################################################## diff --git a/tests/foundation/registry/test_registrar_code_check_unit.py b/tests/foundation/registry/test_registrar_code_check_unit.py index 16d064f429..a3e60c6bdd 100644 --- a/tests/foundation/registry/test_registrar_code_check_unit.py +++ b/tests/foundation/registry/test_registrar_code_check_unit.py @@ -236,13 +236,51 @@ def test_number_registrars_reject_a_bool(self) -> None: """A :class:`bool` is an :class:`int` subclass, but never a port or PPID (#2269).""" from pcapkit.foundation.registry import protocols as registrars from pcapkit.protocols.misc.raw import Raw + from pcapkit.protocols.transport.tcp import TCP from pcapkit.utilities.exceptions import RegistryError for name, _, targets, extra in NUMBER_REGISTRARS: self._assert_rejected_unchanged(name, tuple(map(resolve, targets)), Raw, *extra, codes=(True, False)) - with self.assertRaisesRegex(RegistryError, r'^code must be an int or a member of AppType, not True$'): - registrars._check_code(True, registrars.Enum_AppType, number=True) + with self.assertRaisesRegex(RegistryError, r'^TCP: code must be an int or a member of AppType, not True$'): + registrars._check_code(True, registrars.Enum_AppType, TCP, number=True) + + def test_every_rejection_names_the_destination_registry(self) -> None: + """The message says *whose* registry rejected the key (#2268). + + Each registrar hands + :func:`~pcapkit.foundation.registry.protocols._check_code` the protocol + class whose registry the key is destined for, and the shared message + leads with its name -- several registrars are typed on the same enum, so + the enum alone does not say which one refused. The class expected here is + the first one listed above for that registrar, which for the two + registrars writing into a pair of registries keyed alike + (``register_linktype``, ``register_apptype``) is the one the check names. + """ + from pcapkit.const.reg.linktype import LinkType + from pcapkit.foundation.registry import protocols as registrars + from pcapkit.protocols.misc.raw import Raw + from pcapkit.utilities.exceptions import RegistryError + + def parser(*args: 'object', **kwargs: 'object') -> None: ... + + cases = [] # type: list[tuple[str, str, object, tuple[object, ...], dict[str, object]]] + for name, proto, schema in METHOD_REGISTRARS: + cases.append((name, resolve(proto).__name__, self._wrong_code(name), + ((parser, parser),), {'schema': resolve(schema)})) + for name, targets in MODULE_REGISTRARS: + cases.append((name, resolve(targets[0]).__name__, self._wrong_code(name), (Raw,), {})) + for name, number, targets, extra in NUMBER_REGISTRARS: + # these four take a plain int, so only a member of another enum is wrong + cases.append((name, resolve(targets[0]).__name__, LinkType(number), + (Raw, *extra), {})) + + for name, expected, code, args, kwargs in cases: + with self.subTest(registrar=name): + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + with self.assertRaisesRegex(RegistryError, rf'^{expected}: code must be '): + getattr(registrars, name)(code, *args, **kwargs) if __name__ == '__main__': diff --git a/tests/protocols/test_protocol_code_registration_unit.py b/tests/protocols/test_protocol_code_registration_unit.py index 0341327308..cc757f6904 100644 --- a/tests/protocols/test_protocol_code_registration_unit.py +++ b/tests/protocols/test_protocol_code_registration_unit.py @@ -525,36 +525,37 @@ class FakeProtocol: reg_loopback.assert_called_once_with(2, FakeProtocol) def test_every_destination_kind_row_is_enforced(self) -> None: - """Every row of ``_DESTINATION_KINDS`` is exercised, both cells of it. + """Every ``__proto_key__`` declaration is exercised, both cells of it. - The table is what the check is made of, so a row dropped from it, or a - ``kind``/``number`` cell changed in it, silently stops checking a - destination -- the registrars it stands in for are not called here, so - nothing else would notice. Each row is pinned by behaviour rather than - by comparing the table with a copy of itself: + The declarations are what the check is made of, so one dropped from a + class, or a ``kind``/``number`` cell changed in it, silently stops + checking a destination -- the registrars they stand in for are not + called here, so nothing else would notice. Each is pinned by behaviour + rather than by comparing the declaration with a copy of itself: * ``kind`` -- a member of *another* registry's enum must be rejected, and must leave every registry as it was. That is the #2224 confusion - the check exists for, so a row that stops being checked, or one + the check exists for, so a class that stops being checked, or one checked against the wrong enum, fails here. * ``number`` -- a bare :class:`int` is accepted where the registrar accepts one (a port, a payload protocol identifier) and rejected where it does not, which is the only observable difference the flag - makes. The :class:`int` given to a rejecting row is the *value* of a + makes. The :class:`int` given to a rejecting class is the *value* of a member its registry already ships an entry at, so the check is pinned as being on the type and the entry at risk is a real one. - The ``kind`` identity is asserted as well, since a row retyped to an - enum unrelated to every key used here would reject both keys for the - wrong reason. + The declared pairing is asserted as well, since one retyped to an enum + unrelated to every key used here would reject both keys for the wrong + reason. It is read out of the class's own :func:`vars`, which is where + :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` + reads it: an inherited one would not be its own declaration. """ from pcapkit.const.reg.apptype import AppType from pcapkit.const.reg.ethertype import EtherType from pcapkit.const.reg.linktype import LinkType from pcapkit.const.reg.transtype import TransType from pcapkit.const.sctp.payload_protocol_identifier import PayloadProtocolIdentifier - from pcapkit.foundation.registry.protocols import (_DESTINATION_KINDS, - register_protocol_code) + from pcapkit.foundation.registry.protocols import register_protocol_code from pcapkit.protocols.internet.internet import Internet from pcapkit.protocols.link.link import Link from pcapkit.protocols.misc.pcap.frame import Frame @@ -581,7 +582,7 @@ def test_every_destination_kind_row_is_enforced(self) -> None: for destination, (kind, number, member, raw) in expected.items(): with self.subTest(destination=destination.__name__): - self.assertNotIsInstance(member, kind) # the test data, not the table + self.assertNotIsInstance(member, kind) # the test data, not the declaration self._assert_rejected_unchanged( lambda dest=destination, key=member: register_protocol_code(Raw, {dest: key})) @@ -594,11 +595,86 @@ def test_every_destination_kind_row_is_enforced(self) -> None: self._assert_rejected_unchanged( lambda dest=destination, key=raw: register_protocol_code(Raw, {dest: key})) - self.assertIs(_DESTINATION_KINDS.get(destination, (None, None))[0], kind, - f'{destination.__name__} is not checked against {kind.__name__}') + self.assertEqual(vars(destination).get('__proto_key__'), (kind, number), + f'{destination.__name__} does not declare ' + f'({kind.__name__}, {number})') + + self.assertEqual(self._declaring_classes(), set(expected), + '__proto_key__ is no longer declared by the classes cased here') + + @staticmethod + def _declaring_classes() -> 'set[type]': + """Every ``ProtocolBase`` descendant declaring a ``__proto_key__`` of its own.""" + import pcapkit # noqa: F401 # pylint: disable=unused-import + from pcapkit.protocols.protocol import ProtocolBase + + found = set() + pending = [ProtocolBase] + while pending: + klass = pending.pop() + if '__proto_key__' in vars(klass): + found.add(klass) + pending.extend(klass.__subclasses__()) + return found + + def test_a_registry_of_its_own_does_not_inherit_its_parent_pairing(self) -> None: + """``Loopback.__proto__`` is keyed by address family, ``IPX.__proto__`` by packet type. + + The pairing is read from the owning class's own :func:`vars`, not with + :func:`getattr`, which walks the MRO: :class:`~pcapkit.protocols.link.\ +loopback.Loopback` subclasses :class:`~pcapkit.protocols.link.link.Link` and + :class:`~pcapkit.protocols.internet.ipx.IPX` subclasses + :class:`~pcapkit.protocols.internet.internet.Internet`, so each + *inherits* the attribute while owning a registry keyed by something + else entirely. A key is therefore registered here rather than only the + absent declaration asserted: checking address family ``2`` against + :class:`~pcapkit.const.reg.ethertype.EtherType`, or an + :class:`~pcapkit.const.ipx.packet.Packet` member against + :class:`~pcapkit.const.reg.transtype.TransType`, rejects every key + these registries do accept, and an assertion on + :func:`vars` alone passes either way. + """ + from pcapkit.const.ipx.packet import Packet + from pcapkit.foundation.registry.protocols import register_protocol_code + from pcapkit.protocols.internet.internet import Internet + from pcapkit.protocols.internet.ipx import IPX + from pcapkit.protocols.link.link import Link + from pcapkit.protocols.link.loopback import Loopback - self.assertEqual(set(_DESTINATION_KINDS), set(expected), - '_DESTINATION_KINDS no longer lists the destinations cased here') + cases = ( + (Loopback, Link, 2), # an address family, not an EtherType + (IPX, Internet, 4), # a bare packet type, not an IP protocol number + (IPX, Internet, Packet.PEP), # the same, as a member of its own enum + ) + for destination, parent, key in cases: + with self.subTest(destination=destination.__name__, key=key): + self.assertIn('__proto__', vars(destination)) # it owns a registry + self.assertNotIn('__proto_key__', vars(destination)) # declares no pairing + # inherited, not declared -- which is what makes the ``vars()`` read load-bearing + self.assertEqual(destination.__proto_key__, parent.__proto_key__) + + class FakeProtocol: + pass + + with mock.patch.object(destination, 'register') as register: + register_protocol_code(FakeProtocol, {destination: key}) + register.assert_called_once_with(key, FakeProtocol) + + def test_a_rejection_names_the_destination_registry(self) -> None: + """The message says *whose* registry rejected the key, not only what it wanted.""" + from pcapkit.const.reg.linktype import LinkType + from pcapkit.const.reg.transtype import TransType + from pcapkit.foundation.registry.protocols import register_protocol_code + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.raw import Raw + from pcapkit.protocols.transport.udp import UDP + from pcapkit.utilities.exceptions import RegistryError + + for destination, key in ((Frame, TransType.ICMP), (UDP, LinkType(53))): + with self.subTest(destination=destination.__name__): + with self.assertRaisesRegex(RegistryError, + rf'^{destination.__name__}: code must be '): + register_protocol_code(Raw, {destination: key}) if __name__ == '__main__':