fix(registry): re-registering a stored ModuleDescriptor keeps it (#1364)

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)

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)

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)

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)

pcapkit/protocols/protocol.py

@@ -904,22 +904,27 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor | Type[ProtocolBase]'
             The guard matches :func:`register_protocol
             <pcapkit.foundation.registry.protocols.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)

pcapkit/protocols/transport/sctp.py

@@ -618,16 +618,22 @@ def register(cls, code: 'Enum_PayloadProtocolIdentifier | int', protocol: 'Modul
                 <pcapkit.protocols.transport.transport.Transport.register>` 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)

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)

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()