From 584d5b02a09581085e4bcd989a15dde950147db8 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 8 Oct 2026 09:22:52 -0400 Subject: [PATCH] fix(registry): re-registering a stored ModuleDescriptor keeps it (#1364) - ProtocolBase.register and the Internet, Link, Transport, SCTP, Frame and PCAPNG overrides resolved a ModuleDescriptor argument to its class before comparing it with the stored entry, so handing back the stored descriptor warned and replaced the deferred entry with the class. They now compare first: the stored entry, or a descriptor equal to it, is a silent no-op that keeps the stored object. A different entry still warns, and invalid input still raises RegistryError. - Extractor.register_dumper already keeps its shipped descriptors since #1376; it is pinned in the new test. New tests/protocols/test_registry_stored_descriptor_unit.py: 6 tests; 8 fail on origin/main (7 subtests and the ProtocolBase case). --- pcapkit/protocols/internet/internet.py | 6 +- pcapkit/protocols/link/link.py | 6 +- pcapkit/protocols/misc/pcap/frame.py | 6 +- pcapkit/protocols/misc/pcapng.py | 9 +- pcapkit/protocols/protocol.py | 21 ++- pcapkit/protocols/transport/sctp.py | 10 +- pcapkit/protocols/transport/transport.py | 6 +- .../test_registry_stored_descriptor_unit.py | 141 ++++++++++++++++++ 8 files changed, 189 insertions(+), 16 deletions(-) create mode 100644 tests/protocols/test_registry_stored_descriptor_unit.py diff --git a/pcapkit/protocols/internet/internet.py b/pcapkit/protocols/internet/internet.py index f6514b0ed1..e9a9c40506 100644 --- a/pcapkit/protocols/internet/internet.py +++ b/pcapkit/protocols/internet/internet.py @@ -157,13 +157,17 @@ def register(cls, code: 'Enum_TransType', protocol: 'ModuleDescriptor[ProtocolBa guard this shares with ``register_protocol``. """ + incumbent = cls.__proto__.get(code) + if incumbent is not None and (incumbent is protocol or ( + isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor) + and incumbent == protocol)): + return if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not isinstance(protocol, type): raise RegistryError(f'protocol must be a class, not {protocol!r}') if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - incumbent = cls.__proto__.get(code) if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' f'{incumbent!r} with {protocol!r}', RegistryWarning) diff --git a/pcapkit/protocols/link/link.py b/pcapkit/protocols/link/link.py index 682dcb7eb5..081238c276 100644 --- a/pcapkit/protocols/link/link.py +++ b/pcapkit/protocols/link/link.py @@ -138,13 +138,17 @@ def register(cls, code: 'Enum_EtherType', protocol: 'ModuleDescriptor[ProtocolBa guard this shares with ``register_protocol``. """ + incumbent = cls.__proto__.get(code) + if incumbent is not None and (incumbent is protocol or ( + isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor) + and incumbent == protocol)): + return if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not isinstance(protocol, type): raise RegistryError(f'protocol must be a class, not {protocol!r}') if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - incumbent = cls.__proto__.get(code) if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' f'{incumbent!r} with {protocol!r}', RegistryWarning) diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index 493e2ab24c..86f7041fbf 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -146,13 +146,17 @@ def register(cls, code: 'Enum_LinkType', protocol: 'ModuleDescriptor[ProtocolBas guard this shares with ``register_protocol``. """ + incumbent = cls.__proto__.get(code) + if incumbent is not None and (incumbent is protocol or ( + isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor) + and incumbent == protocol)): + return if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not isinstance(protocol, type): raise RegistryError(f'protocol must be a class, not {protocol!r}') if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - incumbent = cls.__proto__.get(code) if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' f'{incumbent!r} with {protocol!r}', RegistryWarning) diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index 7bd027d667..11c6e60a36 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -996,18 +996,23 @@ def register(cls, code: 'Enum_LinkType', protocol: 'ModuleDescriptor[ProtocolBas entry and its replacement so a caller can tell *what* was lost. Fires only when the incumbent differs from the replacement, and an unresolved module descriptor counts as different from the - class it names. This registry is separate from the PCAP one, so + class it names; handing back the stored entry, or a descriptor + equal to it, is a silent no-op that keeps it. This registry is separate from the PCAP one, so :func:`~pcapkit.foundation.registry.protocols.register_linktype` writing to both cannot make either warn about the other. """ + incumbent = cls.__proto__.get(code) + if incumbent is not None and (incumbent is protocol or ( + isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor) + and incumbent == protocol)): + return if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not isinstance(protocol, type): raise RegistryError(f'protocol must be a class, not {protocol!r}') if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - incumbent = cls.__proto__.get(code) if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' f'{incumbent!r} with {protocol!r}', RegistryWarning) diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index d685aea8c5..09741b290e 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -904,22 +904,27 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor | Type[ProtocolBase]' The guard matches :func:`register_protocol `'s: it fires only when the incumbent differs from the replacement, so - re-registering the exact same class object under the same ``code`` - is a silent no-op rather than a warning about nothing displaced. - The comparison is a plain ``is`` that resolves nothing, so an - incumbent left as an unresolved - :class:`~pcapkit.corekit.module.ModuleDescriptor` still reports as - different from the class it names, and the deferred import stays - deferred. + handing back the entry already stored under ``code`` -- the same + class object, or a :class:`~pcapkit.corekit.module.ModuleDescriptor` + equal to the stored one -- is a silent no-op rather than a warning + about nothing displaced, and the stored entry is kept as it is + (:issue:`1364`). The comparison runs before ``protocol`` is + resolved and resolves nothing, so a stored descriptor stays + deferred, and an incumbent left as an unresolved descriptor still + reports as different from the class it names. """ + incumbent = cls.__proto__.get(code) + if incumbent is not None and (incumbent is protocol or ( + isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor) + and incumbent == protocol)): + return if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not isinstance(protocol, type): raise RegistryError(f'protocol must be a class, not {protocol!r}') if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - incumbent = cls.__proto__.get(code) if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' f'{incumbent!r} with {protocol!r}', RegistryWarning) diff --git a/pcapkit/protocols/transport/sctp.py b/pcapkit/protocols/transport/sctp.py index fa0d3f075d..683fedaae4 100644 --- a/pcapkit/protocols/transport/sctp.py +++ b/pcapkit/protocols/transport/sctp.py @@ -618,16 +618,22 @@ def register(cls, code: 'Enum_PayloadProtocolIdentifier | int', protocol: 'Modul ` it overrides, an unresolved :class:`~pcapkit.corekit.module.ModuleDescriptor` incumbent - counts as different from the class it names. + counts as different from the class it names; handing back the + stored entry, or a descriptor equal to it, is a silent no-op + that keeps it. """ + incumbent = cls.__proto__.get(code) + if incumbent is not None and (incumbent is protocol or ( + isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor) + and incumbent == protocol)): + return if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not isinstance(protocol, type): raise RegistryError(f'protocol must be a class, not {protocol!r}') if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - incumbent = cls.__proto__.get(code) if incumbent is not None and incumbent is not protocol: warn(f'payload protocol identifier {code} already registered, overwriting ' f'{incumbent!r} with {protocol!r}', RegistryWarning) diff --git a/pcapkit/protocols/transport/transport.py b/pcapkit/protocols/transport/transport.py index 5c58ad56b3..b18d99a8ce 100644 --- a/pcapkit/protocols/transport/transport.py +++ b/pcapkit/protocols/transport/transport.py @@ -112,13 +112,17 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor[ProtocolBase] | Type[ if cls is Transport: raise UnsupportedCall(f'{cls.__name__} is an abstract class') + incumbent = cls.__proto__.get(code) + if incumbent is not None and (incumbent is protocol or ( + isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor) + and incumbent == protocol)): + return if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not isinstance(protocol, type): raise RegistryError(f'protocol must be a class, not {protocol!r}') if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - incumbent = cls.__proto__.get(code) if incumbent is not None and incumbent is not protocol: warn(f'port {code} already registered, overwriting ' f'{incumbent!r} with {protocol!r}', RegistryWarning) diff --git a/tests/protocols/test_registry_stored_descriptor_unit.py b/tests/protocols/test_registry_stored_descriptor_unit.py new file mode 100644 index 0000000000..8e8029cf41 --- /dev/null +++ b/tests/protocols/test_registry_stored_descriptor_unit.py @@ -0,0 +1,141 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #1364: re-registering a stored module descriptor keeps it. + +``ProtocolBase.register`` and the ``Internet``, ``Link``, ``Transport``, +``SCTP`` (keyed by PPID), ``Frame`` and ``PCAPNG`` overrides resolved a +:class:`~pcapkit.corekit.module.ModuleDescriptor` argument to its class before +comparing it with the stored entry. Handing back the stored descriptor itself +therefore warned of an overwrite and replaced the deferred entry with the +class. The registrars now compare first: the stored entry, or a descriptor +equal to it, is a silent no-op that keeps the stored object, while a different +entry still warns. ``Extractor.register_dumper`` is pinned alongside; #1376 +already keeps its shipped descriptors. + +Every registry is process-wide, so each case runs under +:func:`unittest.mock.patch.dict`. Classes are imported inside each test, after +:func:`~tests._support.reimport_once_per_class`. + +""" + +import importlib +import unittest +import warnings +from unittest import mock + +from tests._support import reimport_once_per_class + +#: ``(module, class)`` for each registrar, picked so its ``__proto__`` ships +#: module descriptors. IPv4 and Ethernet reach the ``Internet`` and ``Link`` +#: overrides; there is no class whose populated ``__proto__`` reaches +#: ``ProtocolBase.register`` itself, so that one is exercised on a scratch +#: subclass in its own test. +SITES = ( + ('pcapkit.protocols.internet.ipv4', 'IPv4'), + ('pcapkit.protocols.link.ethernet', 'Ethernet'), + ('pcapkit.protocols.transport.tcp', 'TCP'), + ('pcapkit.protocols.transport.udp', 'UDP'), + ('pcapkit.protocols.transport.sctp', 'SCTP'), + ('pcapkit.protocols.misc.pcap.frame', 'Frame'), + ('pcapkit.protocols.misc.pcapng', 'PCAPNG'), +) + + +class TestRegistryStoredDescriptor(unittest.TestCase): + """Pin when a protocol registrar warns about a stored descriptor.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def _count(self, call, *args) -> int: # type: ignore[no-untyped-def] + """Run ``call(*args)`` and return how many registry warnings it raised.""" + from pcapkit.utilities.warnings import RegistryWarning + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + call(*args) + return sum(issubclass(item.category, RegistryWarning) for item in caught) + + def _each_site(self, check): # type: ignore[no-untyped-def] + """Run ``check(register, registry)`` for every site, each in its own ``subTest``.""" + from pcapkit.corekit.module import ModuleDescriptor + + for module, name in SITES: + cls = getattr(importlib.import_module(module), name) + registry = cls.__proto__ + self.assertTrue(any(isinstance(value, ModuleDescriptor) for value in registry.values()), name) + with self.subTest(site=name), mock.patch.dict(registry): + check(cls.register, registry) + + def test_reregistering_the_stored_descriptor_is_silent(self) -> None: + from pcapkit.corekit.module import ModuleDescriptor + + def check(register, registry): # type: ignore[no-untyped-def] + for code, value in list(registry.items()): + if not isinstance(value, ModuleDescriptor): + continue + self.assertEqual(self._count(register, code, value), 0, code) + self.assertIs(registry[code], value) + # An equal descriptor built afresh is the same entry, and the + # stored object is kept. + self.assertEqual(self._count(register, code, ModuleDescriptor(*value)), 0, code) + self.assertIs(registry[code], value) + self._each_site(check) + + def test_reregistering_the_stored_class_is_silent(self) -> None: + def check(register, registry): # type: ignore[no-untyped-def] + code, value = next(iter(registry.items())) + klass = value.klass + self.assertEqual(self._count(register, code, klass), 1) + self.assertIs(registry[code], klass) + self.assertEqual(self._count(register, code, klass), 0) + self.assertIs(registry[code], klass) + self._each_site(check) + + def test_a_different_entry_still_warns(self) -> None: + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.protocols.misc.raw import Raw + + def check(register, registry): # type: ignore[no-untyped-def] + code = next(iter(registry)) + other = ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw') + self.assertEqual(self._count(register, code, other), 1) + self.assertIs(registry[code], Raw) + self._each_site(check) + + def test_an_invalid_entry_equal_to_a_descriptor_still_raises(self) -> None: + from pcapkit.utilities.exceptions import RegistryError + + def check(register, registry): # type: ignore[no-untyped-def] + code, value = next(iter(registry.items())) + with self.assertRaisesRegex(RegistryError, r'^protocol must be a class, not '): + register(code, tuple(value)) + self.assertIs(registry[code], value) + self._each_site(check) + + def test_protocol_base_register_keeps_the_stored_descriptor(self) -> None: + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.protocols.misc.raw import Raw + + descriptor = ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw') + scratch = type('W1364Raw', (Raw,), {'__proto__': {1: descriptor}}) + self.assertEqual(self._count(scratch.register, 1, descriptor), 0) + self.assertIs(scratch.__proto__[1], descriptor) + self.assertEqual(self._count(scratch.register, 1, ModuleDescriptor(*descriptor)), 0) + self.assertIs(scratch.__proto__[1], descriptor) + other = ModuleDescriptor('pcapkit.protocols.misc.null', 'NoPayload') + self.assertEqual(self._count(scratch.register, 1, other), 1) + self.assertIs(scratch.__proto__[1], other.klass) + + def test_register_dumper_keeps_the_stored_descriptor(self) -> None: + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.foundation.extraction import Extractor + + with mock.patch.dict(Extractor.__output__): + entry = Extractor.__output__['json'] + self.assertIsInstance(entry[0], ModuleDescriptor) + self.assertEqual(self._count(Extractor.register_dumper, 'json', *entry), 0) + self.assertIs(Extractor.__output__['json'][0], entry[0]) + + +if __name__ == '__main__': + unittest.main()